Windows 8 introduced new policy for backlight control by pushing it out to graphics drivers. This appears to have coincided with a range of vendors adding Windows 8 checks to their backlight control code which trigger either awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest thing to do would be to just disable ACPI backlight control entirely if the firmware indicates Windows 8 support, but it's entirely possible that individual graphics drivers might still make use of the ACPI functionality in preference to native control.
The first two patches in this series are picked from other patchesets aimed at solving similar problems. The last simply unregisters ACPI backlight control on Windows 8 systems when using an Intel GPU. Similar code could be added to other drivers, but I'm reluctant to do so without further investigation as to the behaviour of the vendor drivers under Windows.
From: "Lee, Chun-Yi" joeyli.kernel@gmail.com
There have some situation we unregister whole acpi/video driver by downstream driver just want to remove backlight control interface of acpi/video. It caues we lost other functions of acpi/video, e.g. transfer acpi event to input event.
So, this patch add a new function, find_video_unregister_backlight, it provide the interface let downstream driver can tell acpi/video to unregister backlight interface of all acpi video devices. Then we can keep functions of acpi/video but only remove backlight support.
Reference: bko#35622 https://bugzilla.kernel.org/show_bug.cgi?id=35622
v2: Also unregister cooling devices.
Tested-by: Andrzej Krentosz endrjux@gmail.com Cc: Zhang Rui rui.zhang@intel.com Cc: Len Brown lenb@kernel.org Cc: Rafael J. Wysocki rjw@sisk.pl Cc: Carlos Corbacho carlos@strangeworlds.co.uk Cc: Matthew Garrett mjg@redhat.com Cc: Dmitry Torokhov dtor@mail.ru Cc: Corentin Chary corentincj@iksaif.net Cc: Aaron Lu aaron.lu@intel.com Cc: Thomas Renninger trenn@suse.de Signed-off-by: Lee, Chun-Yi jlee@suse.com --- drivers/acpi/video.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++---- include/acpi/video.h | 2 ++ 2 files changed, 89 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 5b32e15..da08ff7 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -43,6 +43,7 @@ #include <acpi/acpi_drivers.h> #include <linux/suspend.h> #include <acpi/video.h> +#include <linux/mutex.h>
#define PREFIX "ACPI: "
@@ -86,6 +87,9 @@ module_param(allow_duplicates, bool, 0644); static bool use_bios_initial_backlight = 1; module_param(use_bios_initial_backlight, bool, 0644);
+static bool backlight_disable; +module_param(backlight_disable, bool, 0644); + static int register_count = 0; static int acpi_video_bus_add(struct acpi_device *device); static int acpi_video_bus_remove(struct acpi_device *device); @@ -214,6 +218,8 @@ static const char device_decode[][30] = { "UNKNOWN", };
+static DEFINE_MUTEX(backlight_mutex); + static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data); static void acpi_video_device_rebind(struct acpi_video_bus *video); static void acpi_video_device_bind(struct acpi_video_bus *video, @@ -882,7 +888,7 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device) device->cap._DDC = 1; }
- if (acpi_video_backlight_support()) { + if (acpi_video_backlight_support() || backlight_disable) { struct backlight_properties props; struct pci_dev *pdev; acpi_handle acpi_parent; @@ -1083,15 +1089,22 @@ acpi_video_bus_get_one_device(struct acpi_device *device, struct acpi_video_device *data; struct acpi_video_device_attrib* attribute;
+ mutex_lock(&backlight_mutex); + status = acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id); /* Some device omits _ADR, we skip them instead of fail */ - if (ACPI_FAILURE(status)) - return 0; + if (ACPI_FAILURE(status)) { + status = 0; + goto out; + }
data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL); - if (!data) - return -ENOMEM; + if (!data) { + status = -ENOMEM; + goto out; + } +
strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME); strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS); @@ -1156,6 +1169,8 @@ acpi_video_bus_get_one_device(struct acpi_device *device, list_add_tail(&data->entry, &video->video_device_list); mutex_unlock(&video->device_list_lock);
+out: + mutex_unlock(&backlight_mutex); return status; }
@@ -1336,7 +1351,7 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event) int result = -EINVAL;
/* no warning message if acpi_backlight=vendor is used */ - if (!acpi_video_backlight_support()) + if (!acpi_video_backlight_support() || backlight_disable) return 0;
if (!device->brightness) @@ -1869,6 +1884,72 @@ static int __init intel_opregion_present(void) return opregion; }
+static acpi_status +find_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; + acpi_status status = AE_OK; + + mutex_lock(&backlight_mutex); + + if (acpi_bus_get_device(handle, &acpi_dev)) + goto out; + + if (!acpi_match_device_ids(acpi_dev, video_device_ids)) { + video = acpi_driver_data(acpi_dev); + + if (!video) + goto out; + + 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); + } +out: + mutex_unlock(&backlight_mutex); + return status; +} + +void acpi_video_backlight_unregister(void) +{ + if (!register_count) { + /* + * If the acpi video bus is already unloaded, don't + * unregister backlight of devices and return directly. + */ + return; + } + + backlight_disable = 1; + + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, + ACPI_UINT32_MAX, find_video_unregister_backlight, + NULL, NULL, NULL); + return; +} +EXPORT_SYMBOL(acpi_video_backlight_unregister); + int acpi_video_register(void) { int result = 0; diff --git a/include/acpi/video.h b/include/acpi/video.h index 61109f2..1e810a1 100644 --- a/include/acpi/video.h +++ b/include/acpi/video.h @@ -19,11 +19,13 @@ struct acpi_device; #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE) extern int acpi_video_register(void); extern void acpi_video_unregister(void); +extern void acpi_video_backlight_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 void acpi_video_unregister(void) { return; } +static inline void acpi_video_backlight_unregister(void) { return; } static inline int acpi_video_get_edid(struct acpi_device *device, int type, int device_id, void **edid) {
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 seth.forshee@canonical.com
Signed-off-by: Matthew Garrett matthew.garrett@nebula.com Cc: Seth Forshee seth.forshee@canonical.com --- drivers/acpi/acpica/aclocal.h | 13 ------------- drivers/acpi/acpica/utxface.c | 19 +++++++++++++++++++ include/acpi/acpixf.h | 22 ++++++++++++++++++++++ 3 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h index d5bfbd3..8a2f532 100644 --- a/drivers/acpi/acpica/aclocal.h +++ b/drivers/acpi/acpica/aclocal.h @@ -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 { diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c index 6505774..c1638c3 100644 --- a/drivers/acpi/acpica/utxface.c +++ b/drivers/acpi/acpica/utxface.c @@ -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 diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index 454881e..41d3ac1 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -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);
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 matthew.garrett@nebula.com --- drivers/gpu/drm/i915/i915_dma.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3b315ba..23b6292 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -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))
On Sun, Jun 09, 2013 at 07:01:39PM -0400, Matthew Garrett wrote:
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 matthew.garrett@nebula.com
Make sense and I guess it's easier to merge this all through the acpi tree. So
Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/i915/i915_dma.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3b315ba..23b6292 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -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
於 日，2013-06-09 於 19:01 -0400，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 matthew.garrett@nebula.com
drivers/gpu/drm/i915/i915_dma.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3b315ba..23b6292 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -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))
This patch set works to me on Acer Aspire V3 notebook for unregister the backlight interface of acpi video driver when i915 loaded. Acer Aspire V3 has the Windows8 support in DSDT.
Tested-by: Lee, Chun-Yi jlee@suse.com
Thanks a lot! Joey Lee
Radeon probably needs something similar. See attached untested patch.
Alex
On Sun, Jun 9, 2013 at 7:01 PM, Matthew Garrett matthew.garrett@nebula.com wrote:
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 matthew.garrett@nebula.com
drivers/gpu/drm/i915/i915_dma.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3b315ba..23b6292 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -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 dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 06/10/2013 07:01 AM, Matthew Garrett wrote:
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 matthew.garrett@nebula.com
drivers/gpu/drm/i915/i915_dma.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3b315ba..23b6292 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -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();
What about the platform driver created backlight interface? Oh right, thinkpad_acpi will not create backlight interface for them since they are not in DMI table, so acpi_video_backlight_support() will return true and thinkspad_acpi thinks ACPI video driver is controlling backlight so it will not create backlight interface for them.
Then we will need to remember not to add any of those systems into the DMI table of video_detect.c, or thinkpad_acpi module will create backlight interface for them and according to reporter, that interface does not work either.
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.
For a complete description on backlight control background and the proposed solution, please take a look at: https://gist.github.com/aaronlu/5779828
It would be good to know your opinions, thanks.
-Aaron
On Fri, 2013-06-14 at 14:47 +0800, Aaron Lu wrote:
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?
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?
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.
On 06/15/2013 01:29 AM, Matthew Garrett wrote:
On Fri, 2013-06-14 at 14:47 +0800, Aaron Lu wrote:
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?
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.
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?
Yes that's the case.
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.
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.
Thanks, Aaron
On Sat, 2013-06-15 at 09:26 +0800, Aaron Lu wrote:
On 06/15/2013 01:29 AM, Matthew Garrett wrote:
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.
Right, that's not a great solution.
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.
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.
On 06/15/2013 09:38 AM, Matthew Garrett wrote:
On Sat, 2013-06-15 at 09:26 +0800, Aaron Lu wrote:
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
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.
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.
The above information and idea is formed while solving bugs reported in kernel bugzilla video component, the proposed solutin may not be good enough, but I hope we can find a better way to handle backlight problems.
Thanks, Aaron
On Sat, Jun 15, 2013 at 12:14:42PM +0800, Aaron Lu wrote:
On 06/15/2013 09:38 AM, Matthew Garrett wrote:
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.
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).
Sure, but it still requires the replacement of existing userspace. We need to fix the problem with existing userspace, too.
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.
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.
On 06/15/2013 12:19 PM, Matthew Garrett wrote:
On Sat, Jun 15, 2013 at 12:14:42PM +0800, Aaron Lu wrote:
On 06/15/2013 09:38 AM, Matthew Garrett wrote:
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.
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.
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).
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.
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.
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?
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.
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?
Thanks, Aaron
On Sat, Jun 15, 2013 at 08:29:15PM +0800, Aaron Lu wrote:
On 06/15/2013 12:19 PM, Matthew Garrett wrote:
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.
On systems where the ACPI interface is broken, the GPU driver clearly doesn't use the ACPI interface. As yet, we don't know if that's always true, though.
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.
The ACPI driver has no way of making this kind of policy decision. The GPU driver does.
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?
I'd prefer to figure out how it works in Windows and implement it the same way.
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.
Sure it is - for all we know there's a value in the opregion space that tells the Intel GPU driver which interface we should be using. We don't know because Intel haven't released a version of the opregion driver newer than 2007. It may be that all Windows 8 GPU drivers use the GPU backlight control registers directly and never use any firmware interface, but right now we simply don't know that. All we can do is look at the existing cases we have and say that it appears that Intel graphics systems don't use the ACPI interface. Anything more than that requires more evidence.
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?
The GPU driver makes the policy decision under Windows 8, so it's the natural place to make this decision on Windows 8 systems.
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
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.
Cheers, Daniel
On Sat, Jun 15, 2013 at 08:29:42PM +0200, Daniel Vetter wrote:
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.
It's only used if there's no backlight property on the display.
On Saturday, June 15, 2013 08:29:42 PM Daniel Vetter wrote:
Hi all,
So to me it looks like the discussion is going in circles a bit, hence let me drop my maintainer-opinion here:
- 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?
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.
- 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.
- 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.
- The currently established priority selection for backlights of platform
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.
- 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.
Agreed.
Thanks, Rafael
On Sat, Jun 15, 2013 at 10:27:06PM +0200, Rafael J. Wysocki wrote:
On Saturday, June 15, 2013 08:29:42 PM Daniel Vetter wrote:
Hi all,
So to me it looks like the discussion is going in circles a bit, hence let me drop my maintainer-opinion here:
- 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?
It has to go through the ACPI tree because of the ACPICA patch that needs to be synchronized with the ACPICA upstream. Sorry.
No problem, since we're pretty close to the merge window that would have at most resulted in a few more weeks of testing in i915 trees anyway. -rc kernels should still give us plenty of time to catch fallout, if there is any.
That said, I'm going to take this patchset.
Thanks, Daniel
On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
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 matthew.garrett@nebula.com
drivers/gpu/drm/i915/i915_dma.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3b315ba..23b6292 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -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))
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.
Thanks, Rafael
On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
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 matthew.garrett@nebula.com
drivers/gpu/drm/i915/i915_dma.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3b315ba..23b6292 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -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))
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.
Thanks, Rafael
On Friday, July 05, 2013 10:00:55 PM Rafael J. Wysocki wrote:
On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
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 matthew.garrett@nebula.com
drivers/gpu/drm/i915/i915_dma.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3b315ba..23b6292 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -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))
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.
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.
Thanks, Rafael
--- drivers/acpi/video_detect.c | 11 +++++++++-- include/linux/acpi.h | 1 + 2 files changed, 10 insertions(+), 2 deletions(-)
Index: linux-pm/drivers/acpi/video_detect.c =================================================================== --- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -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_FORCE_NO_BACKLIGHT; } else { status = acpi_bus_get_device(graphics_handle, &tmp_dev); if (ACPI_FAILURE(status)) { @@ -258,13 +261,17 @@ int acpi_video_backlight_support(void) { acpi_video_caps_check();
- /* First check for boot param -> highest prio */ + /* First, check if no backlight support has been forced upon us. */ + if (acpi_video_support & ACPI_VIDEO_FORCE_NO_BACKLIGHT) + return 0; + + /* Next check for boot param -> second highest prio */ if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR) return 0; else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO) return 1;
- /* Then check for DMI blacklist -> second highest prio */ + /* Then check for DMI blacklist -> third highest prio */ if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VENDOR) return 0; else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VIDEO) Index: linux-pm/include/linux/acpi.h =================================================================== --- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -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_FORCE_NO_BACKLIGHT 0x1000
#if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
On Friday, July 05, 2013 11:40:02 PM Rafael J. Wysocki wrote:
On Friday, July 05, 2013 10:00:55 PM Rafael J. Wysocki wrote:
On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
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 matthew.garrett@nebula.com
drivers/gpu/drm/i915/i915_dma.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3b315ba..23b6292 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -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))
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.
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 =================================================================== --- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -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; } else { status = acpi_bus_get_device(graphics_handle, &tmp_dev); if (ACPI_FAILURE(status)) {
On 07/06/2013 06:23 AM, Rafael J. Wysocki wrote:
On Friday, July 05, 2013 11:40:02 PM Rafael J. Wysocki wrote:
On Friday, July 05, 2013 10:00:55 PM Rafael J. Wysocki wrote:
On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
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 matthew.garrett@nebula.com
drivers/gpu/drm/i915/i915_dma.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3b315ba..23b6292 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -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))
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.
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
--- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -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.
Thanks, Aaron
} 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday, July 06, 2013 01:45:56 PM Aaron Lu wrote:
On 07/06/2013 06:23 AM, Rafael J. Wysocki wrote:
On Friday, July 05, 2013 11:40:02 PM Rafael J. Wysocki wrote:
On Friday, July 05, 2013 10:00:55 PM Rafael J. Wysocki wrote:
On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
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 matthew.garrett@nebula.com
drivers/gpu/drm/i915/i915_dma.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3b315ba..23b6292 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -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))
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.
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
--- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -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.
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.
Thanks, Rafael
On Saturday, July 06, 2013 03:33:01 PM Rafael J. Wysocki wrote:
On Saturday, July 06, 2013 01:45:56 PM Aaron Lu wrote:
On 07/06/2013 06:23 AM, Rafael J. Wysocki wrote:
On Friday, July 05, 2013 11:40:02 PM Rafael J. Wysocki wrote:
On Friday, July 05, 2013 10:00:55 PM Rafael J. Wysocki wrote:
On Friday, July 05, 2013 02:20:14 PM Rafael J. Wysocki wrote:
On Sunday, June 09, 2013 07:01:39 PM Matthew Garrett wrote:
[...]
Actually, I wonder what about
[...]
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
--- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -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.
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 rafael.j.wysocki@intel.com 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 rafael.j.wysocki@intel.com --- 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 =================================================================== --- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -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 =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_dma.c +++ linux-pm/drivers/gpu/drm/i915/i915_dma.c @@ -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 =================================================================== --- linux-pm.orig/include/acpi/video.h +++ linux-pm/include/acpi/video.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -234,6 +234,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_BACKLIGHT_FORCE_VENDOR; + 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 =================================================================== --- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -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) { }
On 07/07/2013 09:19 PM, Rafael J. Wysocki wrote:
OK, the patch is appended. Please have a look and tell me what you think.
Thanks, Rafael
From: Rafael J. Wysocki rafael.j.wysocki@intel.com 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.
The condition thinkpad_acpi checks to decide if it wants to create backlight control interface is acpi_video_backlight_support function, not that if ACPI video driver has registered previously. I'm sorry if my previous words gave you such a conclusion.
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 rafael.j.wysocki@intel.com
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
--- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -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 =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_dma.c +++ linux-pm/drivers/gpu/drm/i915/i915_dma.c @@ -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
--- linux-pm.orig/include/acpi/video.h +++ linux-pm/include/acpi/video.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -234,6 +234,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_BACKLIGHT_FORCE_VENDOR;
If the ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR flag is set, acpi_video_backlight_support will return 0, thinkpad_acpi will create backlight interface for the system, which is not what we want.
Thanks, Aaron
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
--- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -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 rafael.j.wysocki@intel.com
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 rafael.j.wysocki@intel.com --- 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 =================================================================== --- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -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 =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_dma.c +++ linux-pm/drivers/gpu/drm/i915/i915_dma.c @@ -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 =================================================================== --- linux-pm.orig/include/acpi/video.h +++ linux-pm/include/acpi/video.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -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 =================================================================== --- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/internal.h +++ linux-pm/drivers/acpi/internal.h @@ -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_ */
On 07/13/2013 08:46 AM, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki rafael.j.wysocki@intel.com
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 rafael.j.wysocki@intel.com
Reviewed-by: Aaron Lu aaron.lu@intel.com
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.
-Aaron
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
--- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -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 =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_dma.c +++ linux-pm/drivers/gpu/drm/i915/i915_dma.c @@ -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
--- linux-pm.orig/include/acpi/video.h +++ linux-pm/include/acpi/video.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -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
--- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -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
--- linux-pm.orig/drivers/acpi/internal.h +++ linux-pm/drivers/acpi/internal.h @@ -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_ */
On Monday, July 15, 2013 10:36:15 AM Aaron Lu wrote:
On 07/13/2013 08:46 AM, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki rafael.j.wysocki@intel.com
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 rafael.j.wysocki@intel.com
Reviewed-by: Aaron Lu aaron.lu@intel.com
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.
Cool, thanks for testing this!
Can you please also ask bug reporters in the BZ entires related to this to test it too?
Rafael
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
--- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -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 =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_dma.c +++ linux-pm/drivers/gpu/drm/i915/i915_dma.c @@ -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
--- linux-pm.orig/include/acpi/video.h +++ linux-pm/include/acpi/video.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -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
--- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -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
--- linux-pm.orig/drivers/acpi/internal.h +++ linux-pm/drivers/acpi/internal.h @@ -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_ */
On 07/15/2013 07:42 PM, Rafael J. Wysocki wrote:
On Monday, July 15, 2013 10:36:15 AM Aaron Lu wrote:
On 07/13/2013 08:46 AM, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki rafael.j.wysocki@intel.com
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 rafael.j.wysocki@intel.com
Reviewed-by: Aaron Lu aaron.lu@intel.com
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.
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.
-Aaron
Rafael
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
--- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -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 =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_dma.c +++ linux-pm/drivers/gpu/drm/i915/i915_dma.c @@ -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
--- linux-pm.orig/include/acpi/video.h +++ linux-pm/include/acpi/video.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -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
--- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -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
--- linux-pm.orig/drivers/acpi/internal.h +++ linux-pm/drivers/acpi/internal.h @@ -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_ */
On Tuesday, July 16, 2013 11:24:05 AM Aaron Lu wrote:
On 07/15/2013 07:42 PM, Rafael J. Wysocki wrote:
On Monday, July 15, 2013 10:36:15 AM Aaron Lu wrote:
On 07/13/2013 08:46 AM, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki rafael.j.wysocki@intel.com
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 rafael.j.wysocki@intel.com
Reviewed-by: Aaron Lu aaron.lu@intel.com
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.
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.
OK
Thanks, Rafael
On Sat, 2013-07-13 at 02:46 +0200, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki rafael.j.wysocki@intel.com
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 rafael.j.wysocki@intel.com
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
--- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -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 =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_dma.c +++ linux-pm/drivers/gpu/drm/i915/i915_dma.c @@ -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
--- linux-pm.orig/include/acpi/video.h +++ linux-pm/include/acpi/video.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -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
--- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -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
--- linux-pm.orig/drivers/acpi/internal.h +++ linux-pm/drivers/acpi/internal.h @@ -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_ */
I can't build it. Where did I go wrong? drivers/acpi/video_detect.c:239:6: error: redefinition of 'acpi_video_backlight_quirks' bool acpi_video_backlight_quirks(void) ^ In file included from drivers/acpi/video_detect.c:41:0: drivers/acpi/internal.h:174:60: note: previous definition of 'acpi_video_backlight_quirks' was here static inline bool acpi_video_backlight_quirks(void) { return false; } ^ drivers/acpi/video_detect.c: In function 'acpi_video_backlight_quirks': drivers/acpi/video_detect.c:241:6: error: 'acpi_gbl_osi_data' undeclared (first use in this function) if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) { ^ drivers/acpi/video_detect.c:241:6: note: each undeclared identifier is reported only once for each function it appears in drivers/acpi/video_detect.c:241:27: error: 'ACPI_OSI_WIN_8' undeclared (first use in this function) if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) { ^ drivers/acpi/video_detect.c: At top level: drivers/acpi/video_detect.c:295:6: error: redefinition of 'acpi_video_verify_backlight_support' bool acpi_video_verify_backlight_support(void) ^ In file included from drivers/acpi/video_detect.c:41:0: drivers/acpi/internal.h:175:60: note: previous definition of 'acpi_video_verify_backlight_support' was here static inline bool acpi_video_verify_backlight_support(void) { return false; } ^
On Monday, July 15, 2013 05:06:09 PM Igor Gnatenko wrote:
On Sat, 2013-07-13 at 02:46 +0200, Rafael J. Wysocki wrote:
[...]
I can't build it. Where did I go wrong?
Probably nowhere, you tried to build the ACPI video driver as a module, that's all. And you need to apply https://patchwork.kernel.org/patch/2812951/ before.
Please try the appended version (on top of https://patchwork.kernel.org/patch/2812951/).
Thanks, Rafael
--- From: Rafael J. Wysocki rafael.j.wysocki@intel.com Subject: ACPI / video / i915: No 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 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 rafael.j.wysocki@intel.com --- 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 =================================================================== --- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -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 =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_dma.c +++ linux-pm/drivers/gpu/drm/i915/i915_dma.c @@ -1648,7 +1648,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 =================================================================== --- linux-pm.orig/include/acpi/video.h +++ linux-pm/include/acpi/video.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -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 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 =================================================================== --- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/internal.h +++ linux-pm/drivers/acpi/internal.h @@ -164,4 +164,15 @@ struct platform_device; int acpi_create_platform_device(struct acpi_device *adev, const struct acpi_device_id *id);
+/*-------------------------------------------------------------------------- + Video + -------------------------------------------------------------------------- */ +#if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) +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_ */
On Tue, 2013-07-16 at 01:53 +0200, Rafael J. Wysocki wrote:
On Monday, July 15, 2013 05:06:09 PM Igor Gnatenko wrote:
On Sat, 2013-07-13 at 02:46 +0200, Rafael J. Wysocki wrote:
[...]
I can't build it. Where did I go wrong?
Probably nowhere, you tried to build the ACPI video driver as a module, that's all. And you need to apply https://patchwork.kernel.org/patch/2812951/ before.
Please try the appended version (on top of https://patchwork.kernel.org/patch/2812951/).
Thanks, Rafael
Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com
From: Rafael J. Wysocki rafael.j.wysocki@intel.com Subject: ACPI / video / i915: No 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 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 rafael.j.wysocki@intel.com
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
--- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -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 =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_dma.c +++ linux-pm/drivers/gpu/drm/i915/i915_dma.c @@ -1648,7 +1648,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
--- linux-pm.orig/include/acpi/video.h +++ linux-pm/include/acpi/video.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -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 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
--- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -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
--- linux-pm.orig/drivers/acpi/internal.h +++ linux-pm/drivers/acpi/internal.h @@ -164,4 +164,15 @@ struct platform_device; int acpi_create_platform_device(struct acpi_device *adev, const struct acpi_device_id *id);
+/*--------------------------------------------------------------------------
Video- -------------------------------------------------------------------------- */
+#if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) +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_ */
Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no longer see switch status of backlight.
On Tue, 2013-07-16 at 17:32 +0400, Igor Gnatenko wrote:
Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no longer see switch status of backlight.
Yeah, I can duplicate that. Rafael, we have to call acpi_video_init_brightness() even if we're not going to initialise the backlight - Thinkpads seem to use this as the trigger for enabling ACPI notifications rather than handling it in firmware. This seems to do the job:
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 01b1a25..71865f7 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -900,6 +900,9 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device) device->cap._DDC = 1; }
+ if (acpi_video_init_brightness(device)) + return; + if (acpi_video_verify_backlight_support()) { struct backlight_properties props; struct pci_dev *pdev; @@ -909,9 +912,6 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device) static int count = 0; char *name;
- result = acpi_video_init_brightness(device); - if (result) - return; name = kasprintf(GFP_KERNEL, "acpi_video%d", count); if (!name) return;
On Tuesday, July 16, 2013 05:08:16 PM Matthew Garrett wrote:
On Tue, 2013-07-16 at 17:32 +0400, Igor Gnatenko wrote:
Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no longer see switch status of backlight.
Yeah, I can duplicate that. Rafael, we have to call acpi_video_init_brightness() even if we're not going to initialise the backlight - Thinkpads seem to use this as the trigger for enabling ACPI notifications rather than handling it in firmware. This seems to do the job:
Igor, does this additional patch from Matthew help?
Rafael
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 01b1a25..71865f7 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -900,6 +900,9 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device) device->cap._DDC = 1; }
- if (acpi_video_init_brightness(device))
return;- if (acpi_video_verify_backlight_support()) { struct backlight_properties props; struct pci_dev *pdev;
@@ -909,9 +912,6 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device) static int count = 0; char *name;
result = acpi_video_init_brightness(device);if (result)return;-- Matthew Garrett | mjg59@srcf.ucam.org
On Wed, 2013-07-17 at 00:01 +0200, Rafael J. Wysocki wrote:
On Tuesday, July 16, 2013 05:08:16 PM Matthew Garrett wrote:
On Tue, 2013-07-16 at 17:32 +0400, Igor Gnatenko wrote:
Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no longer see switch status of backlight.
Yeah, I can duplicate that. Rafael, we have to call acpi_video_init_brightness() even if we're not going to initialise the backlight - Thinkpads seem to use this as the trigger for enabling ACPI notifications rather than handling it in firmware. This seems to do the job:
Igor, does this additional patch from Matthew help?
Yes. With this patch I have backlight switch indicator on my ThinkPad X230.
Rafael
Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 01b1a25..71865f7 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -900,6 +900,9 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device) device->cap._DDC = 1; }
- if (acpi_video_init_brightness(device))
return;- if (acpi_video_verify_backlight_support()) { struct backlight_properties props; struct pci_dev *pdev;
@@ -909,9 +912,6 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device) static int count = 0; char *name;
result = acpi_video_init_brightness(device);if (result)return;-- Matthew Garrett | mjg59@srcf.ucam.org
On Wednesday, July 17, 2013 09:16:38 AM Igor Gnatenko wrote:
On Wed, 2013-07-17 at 00:01 +0200, Rafael J. Wysocki wrote:
On Tuesday, July 16, 2013 05:08:16 PM Matthew Garrett wrote:
On Tue, 2013-07-16 at 17:32 +0400, Igor Gnatenko wrote:
Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no longer see switch status of backlight.
Yeah, I can duplicate that. Rafael, we have to call acpi_video_init_brightness() even if we're not going to initialise the backlight - Thinkpads seem to use this as the trigger for enabling ACPI notifications rather than handling it in firmware. This seems to do the job:
Igor, does this additional patch from Matthew help?
Yes. With this patch I have backlight switch indicator on my ThinkPad X230.
OK, thanks for the confirmation.
Can you please also check if applying the appended patch on top of the Matthew's one changes anything (ie. things still work)?
Rafael
--- drivers/acpi/video.c | 5 +++++ 1 file changed, 5 insertions(+)
Index: linux-pm/drivers/acpi/video.c =================================================================== --- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -957,6 +957,11 @@ static void acpi_video_device_find_cap(s if (result) printk(KERN_ERR PREFIX "Create sysfs link\n");
+ } else { + /* Remove the brightness object. */ + kfree(device->brightness->levels); + kfree(device->brightness); + device->brightness = NULL; } }
On Wed, 2013-07-17 at 13:38 +0200, Rafael J. Wysocki wrote:
On Wednesday, July 17, 2013 09:16:38 AM Igor Gnatenko wrote:
On Wed, 2013-07-17 at 00:01 +0200, Rafael J. Wysocki wrote:
On Tuesday, July 16, 2013 05:08:16 PM Matthew Garrett wrote:
On Tue, 2013-07-16 at 17:32 +0400, Igor Gnatenko wrote:
Hmm. I found regression in user-space. In GNOME (maybe and other DEs) we no longer see switch status of backlight.
Yeah, I can duplicate that. Rafael, we have to call acpi_video_init_brightness() even if we're not going to initialise the backlight - Thinkpads seem to use this as the trigger for enabling ACPI notifications rather than handling it in firmware. This seems to do the job:
Igor, does this additional patch from Matthew help?
Yes. With this patch I have backlight switch indicator on my ThinkPad X230.
OK, thanks for the confirmation.
Can you please also check if applying the appended patch on top of the Matthew's one changes anything (ie. things still work)?
Yes. I've tested and not found regressions in indicator or in switcher. Good work.
Rafael
Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com
drivers/acpi/video.c | 5 +++++ 1 file changed, 5 insertions(+)
Index: linux-pm/drivers/acpi/video.c
--- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -957,6 +957,11 @@ static void acpi_video_device_find_cap(s if (result) printk(KERN_ERR PREFIX "Create sysfs link\n");
- } else {
/* Remove the brightness object. */kfree(device->brightness->levels);kfree(device->brightness);device->brightness = NULL;}
Hi Matthew,
On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote:
Windows 8 introduced new policy for backlight control by pushing it out to graphics drivers. This appears to have coincided with a range of vendors adding Windows 8 checks to their backlight control code which trigger either awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest thing to do would be to just disable ACPI backlight control entirely if the firmware indicates Windows 8 support, but it's entirely possible that individual graphics drivers might still make use of the ACPI functionality in preference to native control.
The first two patches in this series are picked from other patchesets aimed at solving similar problems. The last simply unregisters ACPI backlight control on Windows 8 systems when using an Intel GPU. Similar code could be added to other drivers, but I'm reluctant to do so without further investigation as to the behaviour of the vendor drivers under Windows.
I happen to know that alternative solution to this problem is being worked on, so I'm going to wait until it is submitted and then we'll decide what to merge.
I'm slightly concerned about unregistering ACPI backlight control for all systems declaring win8 support, even though it may actually work for them.
Thanks, Rafael
On Mon, 2013-06-10 at 13:59 +0200, Rafael J. Wysocki wrote:
I happen to know that alternative solution to this problem is being worked on, so I'm going to wait until it is submitted and then we'll decide what to merge.
Sure.
I'm slightly concerned about unregistering ACPI backlight control for all systems declaring win8 support, even though it may actually work for them.
Right, that's why I think it's correct to leave it up to the graphics drivers. It seems like they're the ones that make the policy determination under Windows now. The policy as implemented here may not be correct, but doing better would probably involve Intel letting us know how their Windows driver behaves.
On Sun, Jun 09, 2013 at 07:01:36PM -0400, Matthew Garrett wrote:
Windows 8 introduced new policy for backlight control by pushing it out to graphics drivers. This appears to have coincided with a range of vendors adding Windows 8 checks to their backlight control code which trigger either awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest thing to do would be to just disable ACPI backlight control entirely if the firmware indicates Windows 8 support, but it's entirely possible that individual graphics drivers might still make use of the ACPI functionality in preference to native control.
The first two patches in this series are picked from other patchesets aimed at solving similar problems. The last simply unregisters ACPI backlight control on Windows 8 systems when using an Intel GPU. Similar code could be added to other drivers, but I'm reluctant to do so without further investigation as to the behaviour of the vendor drivers under Windows.
I've received some feedback from user testing of these patches (don't have an affected machine myself) and the feedback is mostly good. One user reported it didn't work, but I that machine may have discrete graphics (waiting to hear back from the user).
The main drawback I see with any approach like this one is that the backlight remains broken for users of proprietary graphics drivers.
Seth
On dim., 2013-06-09 at 19:01 -0400, Matthew Garrett wrote:
The first two patches in this series are picked from other patchesets aimed at solving similar problems. The last simply unregisters ACPI backlight control on Windows 8 systems when using an Intel GPU. Similar code could be added to other drivers, but I'm reluctant to do so without further investigation as to the behaviour of the vendor drivers under Windows.
Hi,
I've read this thread coming from https://bugzilla.kernel.org/show_bug.cgi?id=51231 and tried the patches on a Lenovo ThinkPad X230 with intel graphics.
The problem with thoses fixes is that they still introduce a regression in how the brightness is handled, at least for me.
Before Linux support for acpi_osi("Windows 2012") (and when booting with acpi_osi="!Windows 2012"), brightness keys were handled by the kernel just fine, whether in console, in the display manager or in my desktop environment (Xfce). xfce4-power-manager just needs to be told that the brightness keys are already handled and it doesn't need to do anything.
After Linux started responding to Win8 ACPI checks, it somehow broke, but not completely. Brightness keys are not handled by the kernel anymore (so no brightness adjustment in console or without a hotkey daemon running). If I run xfce4-power-manager and tell it to handle the brightness keys, it does work (although the number of brightness levels is not completely right). That means both acpi_video0 and intel_backlight backlight interfaces work, they just don't have the same precision and max_brightness (more details on the bug).
When adding those three patches, well, acpi_video0 is removed (as intended), but I still don't have brightness handling in the kernel and need to have something handling it in userspace.
I really think this is a bad idea. In some cases it might be the only solution, but having a place where this is set consistently would be really best, imho. Every userspace daemon might do things differently, and not everyone even has it. And I'm really not sure brightness keys should be handled by a desktop environment anyway.
So can the previous behavior be actually restored? Maybe it was easier to pass the brightness keys information from thinkpad_acpi.ko to video.ko than it is to i915.ko but since it goes to the input layer anyway I guess it could be fed to module handling that in a way or another?
Please keep me on CC: on replies, I'm not subscribed to the various lists.
Regards,
On Sat, Jun 22, 2013 at 11:46:39PM +0200, Yves-Alexis Perez wrote:
Before Linux support for acpi_osi("Windows 2012") (and when booting with acpi_osi="!Windows 2012"), brightness keys were handled by the kernel just fine, whether in console, in the display manager or in my desktop environment (Xfce). xfce4-power-manager just needs to be told that the brightness keys are already handled and it doesn't need to do anything.
Right, the kernel has special-casing to hook the backlight keys up to the ACPI backlight control. This is an awful thing, because there's no way to detect this case other than parsing a single driver-specific module parameter.
Could this functionality be duplicated across other backlight drivers? Not easily. The ACPI driver receives keypresses and performs backlight control. The i915 driver doesn't receive keypresses. We could easily tie certain keycodes into backlight events, but which backlight should they control? You're really starting to get into the kind of complex policy decision that's best left to userspace, which is where it should have been to begin with.
On Tue, Jun 25, 2013 at 6:08 PM, Matthew Garrett mjg59@srcf.ucam.org wrote:
On Sat, Jun 22, 2013 at 11:46:39PM +0200, Yves-Alexis Perez wrote:
Before Linux support for acpi_osi("Windows 2012") (and when booting with acpi_osi="!Windows 2012"), brightness keys were handled by the kernel just fine, whether in console, in the display manager or in my desktop environment (Xfce). xfce4-power-manager just needs to be told that the brightness keys are already handled and it doesn't need to do anything.
Right, the kernel has special-casing to hook the backlight keys up to the ACPI backlight control. This is an awful thing, because there's no way to detect this case other than parsing a single driver-specific module parameter.
Could this functionality be duplicated across other backlight drivers? Not easily. The ACPI driver receives keypresses and performs backlight control. The i915 driver doesn't receive keypresses. We could easily tie certain keycodes into backlight events, but which backlight should they control? You're really starting to get into the kind of complex policy decision that's best left to userspace, which is where it should have been to begin with.
Do we have any chances to amend this mistake by (gradually) phasing out support for the direct keypress forwarding in ACPI? Or at least some plans? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Jun 25, 2013 at 06:10:35PM +0200, Daniel Vetter wrote:
Do we have any chances to amend this mistake by (gradually) phasing out support for the direct keypress forwarding in ACPI? Or at least some plans?
We could make the default value of brightness_switch_enabled a config variable and encourage distributions to flip their behaviour.
On mar., 2013-06-25 at 17:08 +0100, Matthew Garrett wrote:
On Sat, Jun 22, 2013 at 11:46:39PM +0200, Yves-Alexis Perez wrote:
Before Linux support for acpi_osi("Windows 2012") (and when booting with acpi_osi="!Windows 2012"), brightness keys were handled by the kernel just fine, whether in console, in the display manager or in my desktop environment (Xfce). xfce4-power-manager just needs to be told that the brightness keys are already handled and it doesn't need to do anything.
Right, the kernel has special-casing to hook the backlight keys up to the ACPI backlight control. This is an awful thing, because there's no way to detect this case other than parsing a single driver-specific module parameter.
I'm not sure what that means. To detect what case exactly? That the brightness is handled by video.ko?
Could this functionality be duplicated across other backlight drivers? Not easily. The ACPI driver receives keypresses and performs backlight control. The i915 driver doesn't receive keypresses. We could easily tie certain keycodes into backlight events, but which backlight should they control? You're really starting to get into the kind of complex policy decision that's best left to userspace, which is where it should have been to begin with.
Well, I get the reasoning, but I'm not sure I agree. That means userspace behavior is inconsistent depending on who does it (gnome-power-manager, gnome-setting-daemon, whatever), and it usually means there's nothing handling the brightness before those are running, not to mention people not running them because they don't run a full blown desktop environment (until someone starts thinking it's a good idea to handle brightness in systemd).
And in the end, the user just want the brightness keys to correctly handle the brightness, full stop. Having multiple brightness daemons using different policies on different hardware is a nightmare for the end user, imho. From a user point of view, having it handled all in the kernel works really pretty fine and is completely transparent (I have to admit that from that point of view, it was even better when it was handled by the EC but those times seem long gone).
Regards,
On Tue, Jun 25, 2013 at 10:43:57PM +0200, Yves-Alexis Perez wrote:
On mar., 2013-06-25 at 17:08 +0100, Matthew Garrett wrote:
Right, the kernel has special-casing to hook the backlight keys up to the ACPI backlight control. This is an awful thing, because there's no way to detect this case other than parsing a single driver-specific module parameter.
I'm not sure what that means. To detect what case exactly? That the brightness is handled by video.ko?
That the kernel will automatically handle backlight key presses.
Could this functionality be duplicated across other backlight drivers? Not easily. The ACPI driver receives keypresses and performs backlight control. The i915 driver doesn't receive keypresses. We could easily tie certain keycodes into backlight events, but which backlight should they control? You're really starting to get into the kind of complex policy decision that's best left to userspace, which is where it should have been to begin with.
Well, I get the reasoning, but I'm not sure I agree. That means userspace behavior is inconsistent depending on who does it (gnome-power-manager, gnome-setting-daemon, whatever), and it usually means there's nothing handling the brightness before those are running, not to mention people not running them because they don't run a full blown desktop environment (until someone starts thinking it's a good idea to handle brightness in systemd).
The behaviour is already inconsistent. If you have an ACPI backlight interface, hitting the keys makes your brightness change without any userspace help. If you don't, it doesn't.
And in the end, the user just want the brightness keys to correctly handle the brightness, full stop. Having multiple brightness daemons using different policies on different hardware is a nightmare for the end user, imho. From a user point of view, having it handled all in the kernel works really pretty fine and is completely transparent (I have to admit that from that point of view, it was even better when it was handled by the EC but those times seem long gone).
I agree, we should standardise the behaviour. And the only way we can standardise the behaviour is to leave it up to userspace.
On mar., 2013-06-25 at 21:54 +0100, Matthew Garrett wrote:
On Tue, Jun 25, 2013 at 10:43:57PM +0200, Yves-Alexis Perez wrote:
On mar., 2013-06-25 at 17:08 +0100, Matthew Garrett wrote:
Right, the kernel has special-casing to hook the backlight keys up to the ACPI backlight control. This is an awful thing, because there's no way to detect this case other than parsing a single driver-specific module parameter.
I'm not sure what that means. To detect what case exactly? That the brightness is handled by video.ko?
That the kernel will automatically handle backlight key presses.
Ok, so for detection by userspace? hal managed to do that just fine, it seems that upower doesn't, for some reason.
The behaviour is already inconsistent. If you have an ACPI backlight interface, hitting the keys makes your brightness change without any userspace help. If you don't, it doesn't.
At least on the same (class of) hardware it always behaves the same.
And in the end, the user just want the brightness keys to correctly handle the brightness, full stop. Having multiple brightness daemons using different policies on different hardware is a nightmare for the end user, imho. From a user point of view, having it handled all in the kernel works really pretty fine and is completely transparent (I have to admit that from that point of view, it was even better when it was handled by the EC but those times seem long gone).
I agree, we should standardise the behaviour. And the only way we can standardise the behaviour is to leave it up to userspace.
It's pretty clear we disagree on this and that my opinion won't really matter here. But letting userspace handle that just means broken functionality for those who have the chance (apparently) to have an ACPI backlight interface.
Regards,
On Tue, Jun 25, 2013 at 11:10:11PM +0200, Yves-Alexis Perez wrote:
On mar., 2013-06-25 at 21:54 +0100, Matthew Garrett wrote:
I agree, we should standardise the behaviour. And the only way we can standardise the behaviour is to leave it up to userspace.
It's pretty clear we disagree on this and that my opinion won't really matter here. But letting userspace handle that just means broken functionality for those who have the chance (apparently) to have an ACPI backlight interface.
Which, as we've already established, you don't - Lenovo broke it. Your Thinkpad claims to have 100 available levels, and most of them don't work. The kernel has no way of knowing which levels work and which don't, so leaving this up to the kernel won't actually fix your system either.
On mar., 2013-06-25 at 22:14 +0100, Matthew Garrett wrote:
On Tue, Jun 25, 2013 at 11:10:11PM +0200, Yves-Alexis Perez wrote:
On mar., 2013-06-25 at 21:54 +0100, Matthew Garrett wrote:
I agree, we should standardise the behaviour. And the only way we can standardise the behaviour is to leave it up to userspace.
It's pretty clear we disagree on this and that my opinion won't really matter here. But letting userspace handle that just means broken functionality for those who have the chance (apparently) to have an ACPI backlight interface.
Which, as we've already established, you don't - Lenovo broke it. Your Thinkpad claims to have 100 available levels, and most of them don't work. The kernel has no way of knowing which levels work and which don't, so leaving this up to the kernel won't actually fix your system either.
I was referring to “standardize the behaviour by leaving up to userspace”. A lot of thinkpads (for example) (all the pre-windows 8 ones) have a perfectly working ACPI backlight interface.
Also, if the kernel has no way of knowing which levels work, I fail to see how userspace can do better.
I understand that switching to intel_backlight instead of acpi_video0 follows what Windows 8 recommends but for me it looks orthogonal to the fact ACPI methods now have some awkward (Lenovo) or broken (Dell). I mean, it's not the first time firmware people break some kernel behavior. I know it's usually not easy to contact them, but shouldn't those methods be fixed, instead of somehow blindly switching to graphic drivers?
On Tue, Jun 25, 2013 at 11:30:37PM +0200, Yves-Alexis Perez wrote:
On mar., 2013-06-25 at 22:14 +0100, Matthew Garrett wrote:
Which, as we've already established, you don't - Lenovo broke it. Your Thinkpad claims to have 100 available levels, and most of them don't work. The kernel has no way of knowing which levels work and which don't, so leaving this up to the kernel won't actually fix your system either.
I was referring to “standardize the behaviour by leaving up to userspace”. A lot of thinkpads (for example) (all the pre-windows 8 ones) have a perfectly working ACPI backlight interface.
And this patchset won't alter their behaviour.
Also, if the kernel has no way of knowing which levels work, I fail to see how userspace can do better.
It can't. That's why this patchset disables the ACPI interface on Windows 8 systems.
I understand that switching to intel_backlight instead of acpi_video0 follows what Windows 8 recommends but for me it looks orthogonal to the fact ACPI methods now have some awkward (Lenovo) or broken (Dell). I mean, it's not the first time firmware people break some kernel behavior. I know it's usually not easy to contact them, but shouldn't those methods be fixed, instead of somehow blindly switching to graphic drivers?
No. The correct answer to all firmware issues is "Are we making the same firmware calls as the version of Windows that this hardware thinks it's running". If Windows 8 doesn't make these calls, we shouldn't make these calls.
On mar., 2013-06-25 at 22:33 +0100, Matthew Garrett wrote:
I was referring to “standardize the behaviour by leaving up to userspace”. A lot of thinkpads (for example) (all the pre-windows 8 ones) have a perfectly working ACPI backlight interface.
And this patchset won't alter their behaviour.
Sorry if I was unclear and if my mail implied that. It was about your remark later in the thread (and the mail from Daniel Vetter)
Also, if the kernel has no way of knowing which levels work, I fail to see how userspace can do better.
It can't. That's why this patchset disables the ACPI interface on Windows 8 systems.
I understand that switching to intel_backlight instead of acpi_video0 follows what Windows 8 recommends but for me it looks orthogonal to the fact ACPI methods now have some awkward (Lenovo) or broken (Dell). I mean, it's not the first time firmware people break some kernel behavior. I know it's usually not easy to contact them, but shouldn't those methods be fixed, instead of somehow blindly switching to graphic drivers?
No. The correct answer to all firmware issues is "Are we making the same firmware calls as the version of Windows that this hardware thinks it's running". If Windows 8 doesn't make these calls, we shouldn't make these calls.
But if that introduce regressions, shouldn't workarounds be found then? Sorry if I keep repeating that but brightness keys handling in-kernel is quite a useful feature and losing it (because of the “behave exactly like Windows 8 kernel” policy) is indeed a regression.
On Tue, Jun 25, 2013 at 11:46:01PM +0200, Yves-Alexis Perez wrote:
But if that introduce regressions, shouldn't workarounds be found then? Sorry if I keep repeating that but brightness keys handling in-kernel is quite a useful feature and losing it (because of the “behave exactly like Windows 8 kernel” policy) is indeed a regression.
Your firmware behaves differently depending on whether the OS claims to be Windows 8 or not. We can't make that invisible.
On Sat, Jun 22, 2013 at 4:46 PM, Yves-Alexis Perez corsac@debian.org wrote:
On dim., 2013-06-09 at 19:01 -0400, Matthew Garrett wrote:
The first two patches in this series are picked from other patchesets aimed at solving similar problems. The last simply unregisters ACPI backlight control on Windows 8 systems when using an Intel GPU. Similar code could be added to other drivers, but I'm reluctant to do so without further investigation as to the behaviour of the vendor drivers under Windows.
Hi,
I've read this thread coming from https://bugzilla.kernel.org/show_bug.cgi?id=51231 and tried the patches on a Lenovo ThinkPad X230 with intel graphics.
The problem with thoses fixes is that they still introduce a regression in how the brightness is handled, at least for me.
For me too.
Before Linux support for acpi_osi("Windows 2012") (and when booting with acpi_osi="!Windows 2012"), brightness keys were handled by the kernel just fine, whether in console, in the display manager or in my desktop environment (Xfce). xfce4-power-manager just needs to be told that the brightness keys are already handled and it doesn't need to do anything.
How do you tell xfce4-power-manager that?
For me everything works fine when acpi_osi="!Windows 2012", which is why I wrote a patch for this particular laptop.
http://article.gmane.org/gmane.linux.acpi.devel/60969
So can the previous behavior be actually restored?
It *should*. The #1 rule of the Linux kernel is to never ever break user-space, isn't it?
Please keep me on CC: on replies, I'm not subscribed to the various lists.
You don't need to ask that in mailing lists that don't have reply-to munged (sane ones), and vger ones don't.
Cheers.
On mer., 2013-07-17 at 10:51 -0500, Felipe Contreras wrote:
On Sat, Jun 22, 2013 at 4:46 PM, Yves-Alexis Perez corsac@debian.org wrote:
Before Linux support for acpi_osi("Windows 2012") (and when booting with acpi_osi="!Windows 2012"), brightness keys were handled by the kernel just fine, whether in console, in the display manager or in my desktop environment (Xfce). xfce4-power-manager just needs to be told that the brightness keys are already handled and it doesn't need to do anything.
How do you tell xfce4-power-manager that?
xfconf-query -c xfce4-power-manager -p /xfce4-power-manager/change-brightness-on-key-events -s false
You might have to create the key before. See https://bugzilla.xfce.org/show_bug.cgi?id=7541 for more information.
Regards,
On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote:
Windows 8 introduced new policy for backlight control by pushing it out to graphics drivers. This appears to have coincided with a range of vendors adding Windows 8 checks to their backlight control code which trigger either awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest thing to do would be to just disable ACPI backlight control entirely if the firmware indicates Windows 8 support, but it's entirely possible that individual graphics drivers might still make use of the ACPI functionality in preference to native control.
The first two patches in this series are picked from other patchesets aimed at solving similar problems. The last simply unregisters ACPI backlight control on Windows 8 systems when using an Intel GPU. Similar code could be added to other drivers, but I'm reluctant to do so without further investigation as to the behaviour of the vendor drivers under Windows.
Well, after some more time spent on that, we now have a series of 3 patches (different from the $subject one) that we think may be used to address this issue. As far as I can say, it has been tested by multiple people whose systems have those problems and they generally saw improvement.
It is not my ideal approach, but it seems to be the least intrusive and/or with the least amount of possible side effects that we can do right now as a general measure (alternatively, we could create a possibly long blacklist table of affected systems with different workarounds for them, but let's just say that is not overwhelmingly attractive).
[1/3] Make ACPICA export things that we need for checking OSI(Win8).
[2/3] Make acpi_video_device_find_cap() call acpi_video_init_brightness() even if it is not going to register the backlight interface (needed for Thinkpads).
[3/3] Avoid using ACPI backlight if i915 is in use and the firmware believes we are Windows 8.
Many thanks to everyone involved!
Rafael
From: Aaron Lu aaron.lu@intel.com
Expose acpi_gbl_osi_data so that code outside of ACPICA can check the value of the last successfull _OSI call. The definitions for OSI versions are moved to actypes.h so that other components can access them too.
Based on a patch from Matthew Garrett which in turn was based on an earlier patch from Seth Forshee.
[rjw: Changelog] Signed-off-by: Aaron Lu aaron.lu@intel.com Signed-off-by: Rafael J. Wysocki rafael.j.wysocki@intel.com --- drivers/acpi/acpica/aclocal.h | 13 ------------- include/acpi/acpixf.h | 1 + include/acpi/actypes.h | 15 +++++++++++++++ 3 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h index dfed265..d4a49016 100644 --- a/drivers/acpi/acpica/aclocal.h +++ b/drivers/acpi/acpica/aclocal.h @@ -931,19 +931,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 { diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index 1b09300..22d497e 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -62,6 +62,7 @@ extern u32 acpi_current_gpe_count; extern struct acpi_table_fadt acpi_gbl_FADT; extern u8 acpi_gbl_system_awake_and_running; extern u8 acpi_gbl_reduced_hardware; /* ACPI 5.0 */ +extern u8 acpi_gbl_osi_data;
/* Runtime configuration of debug print levels */
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index a64adcc..22b03c9 100644 --- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -1144,4 +1144,19 @@ struct acpi_memory_list { #endif };
+/* Definitions for _OSI support */ + +#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 + #endif /* __ACTYPES_H__ */
From: Aaron Lu aaron.lu@intel.com
Expose acpi_gbl_osi_data so that code outside of ACPICA can check the value of the last successfull _OSI call. The definitions for OSI versions are moved to actypes.h so that other components can access them too.
Based on a patch from Matthew Garrett which in turn was based on an earlier patch from Seth Forshee.
[rjw: Changelog] Signed-off-by: Aaron Lu aaron.lu@intel.com Signed-off-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com --- drivers/acpi/acpica/aclocal.h | 13 ------------- include/acpi/acpixf.h | 1 + include/acpi/actypes.h | 15 +++++++++++++++ 3 files changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h index dfed265..d4a49016 100644 --- a/drivers/acpi/acpica/aclocal.h +++ b/drivers/acpi/acpica/aclocal.h @@ -931,19 +931,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 { diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index 1b09300..22d497e 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -62,6 +62,7 @@ extern u32 acpi_current_gpe_count; extern struct acpi_table_fadt acpi_gbl_FADT; extern u8 acpi_gbl_system_awake_and_running; extern u8 acpi_gbl_reduced_hardware; /* ACPI 5.0 */ +extern u8 acpi_gbl_osi_data;
/* Runtime configuration of debug print levels */
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index a64adcc..22b03c9 100644 --- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -1144,4 +1144,19 @@ struct acpi_memory_list { #endif };
+/* Definitions for _OSI support */ + +#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 + #endif /* __ACTYPES_H__ */
From: Matthew Garrett matthew.garrett@nebula.com
We have to call acpi_video_init_brightness() even if we're not going to initialise the backlight - Thinkpads seem to use this as the trigger for enabling ACPI notifications rather than handling it in firmware.
[rjw: Drop the brightness object created by acpi_video_init_brightness() if we are not going to use it.] Signed-off-by: Matthew Garrett matthew.garrett@nebula.com Signed-off-by: Rafael J. Wysocki rafael.j.wysocki@intel.com --- drivers/acpi/video.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
Index: linux-pm/drivers/acpi/video.c =================================================================== --- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -884,6 +884,9 @@ static void acpi_video_device_find_cap(s if (acpi_has_method(device->dev->handle, "_DDC")) device->cap._DDC = 1;
+ if (acpi_video_init_brightness(device)) + return; + if (acpi_video_backlight_support()) { struct backlight_properties props; struct pci_dev *pdev; @@ -893,9 +896,6 @@ static void acpi_video_device_find_cap(s static int count = 0; char *name;
- result = acpi_video_init_brightness(device); - if (result) - return; name = kasprintf(GFP_KERNEL, "acpi_video%d", count); if (!name) return; @@ -955,6 +955,11 @@ static void acpi_video_device_find_cap(s if (result) printk(KERN_ERR PREFIX "Create sysfs link\n");
+ } else { + /* Remove the brightness object. */ + kfree(device->brightness->levels); + kfree(device->brightness); + device->brightness = NULL; } }
From: Matthew Garrett matthew.garrett@nebula.com
We have to call acpi_video_init_brightness() even if we're not going to initialise the backlight - Thinkpads seem to use this as the trigger for enabling ACPI notifications rather than handling it in firmware.
[rjw: Drop the brightness object created by acpi_video_init_brightness() if we are not going to use it.] Signed-off-by: Matthew Garrett matthew.garrett@nebula.com Signed-off-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com --- drivers/acpi/video.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
Index: linux-pm/drivers/acpi/video.c =================================================================== --- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -884,6 +884,9 @@ static void acpi_video_device_find_cap(s if (acpi_has_method(device->dev->handle, "_DDC")) device->cap._DDC = 1;
+ if (acpi_video_init_brightness(device)) + return; + if (acpi_video_backlight_support()) { struct backlight_properties props; struct pci_dev *pdev; @@ -893,9 +896,6 @@ static void acpi_video_device_find_cap(s static int count = 0; char *name;
- result = acpi_video_init_brightness(device); - if (result) - return; name = kasprintf(GFP_KERNEL, "acpi_video%d", count); if (!name) return; @@ -955,6 +955,11 @@ static void acpi_video_device_find_cap(s if (result) printk(KERN_ERR PREFIX "Create sysfs link\n");
+ } else { + /* Remove the brightness object. */ + kfree(device->brightness->levels); + kfree(device->brightness); + device->brightness = NULL; } }
From: Rafael J. Wysocki rafael.j.wysocki@intel.com
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 includes a fix from Aaron Lu's.
References: https://bugzilla.kernel.org/show_bug.cgi?id=51231 Tested-by: Aaron Lu aaron.lu@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com Tested-by: Yves-Alexis Perez corsac@debian.org Signed-off-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Reviewed-by: Aaron Lu aaron.lu@intel.com Acked-by: Matthew Garrett matthew.garrett@nebula.com --- drivers/acpi/internal.h | 11 ++++++ drivers/acpi/video.c | 69 +++++++++++++++++++++++++++++++++++----- 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, 105 insertions(+), 10 deletions(-)
Index: linux-pm/drivers/acpi/video.c =================================================================== --- linux-pm.orig/drivers/acpi/video.c +++ linux-pm/drivers/acpi/video.c @@ -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" @@ -887,7 +889,7 @@ static void acpi_video_device_find_cap(s if (acpi_video_init_brightness(device)) return;
- if (acpi_video_backlight_support()) { + if (acpi_video_verify_backlight_support()) { struct backlight_properties props; struct pci_dev *pdev; acpi_handle acpi_parent; @@ -1334,8 +1336,8 @@ acpi_video_switch_brightness(struct acpi unsigned long long level_current, level_next; int result = -EINVAL;
- /* no warning message if acpi_backlight=vendor is used */ - if (!acpi_video_backlight_support()) + /* no warning message if acpi_backlight=vendor or a quirk is used */ + if (!acpi_video_verify_backlight_support()) return 0;
if (!device->brightness) @@ -1837,6 +1839,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) @@ -1868,14 +1910,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; }
@@ -1891,7 +1944,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 =================================================================== --- linux-pm.orig/drivers/gpu/drm/i915/i915_dma.c +++ linux-pm/drivers/gpu/drm/i915/i915_dma.c @@ -1648,7 +1648,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 =================================================================== --- linux-pm.orig/include/acpi/video.h +++ linux-pm/include/acpi/video.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/video_detect.c +++ linux-pm/drivers/acpi/video_detect.c @@ -38,6 +38,8 @@ #include <linux/dmi.h> #include <linux/pci.h>
+#include "internal.h" + #define PREFIX "ACPI: "
ACPI_MODULE_NAME("video"); @@ -231,6 +233,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 @@ -275,6 +288,14 @@ int acpi_video_backlight_support(void) } EXPORT_SYMBOL(acpi_video_backlight_support);
+/* For the ACPI video driver 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 =================================================================== --- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -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 =================================================================== --- linux-pm.orig/drivers/acpi/internal.h +++ linux-pm/drivers/acpi/internal.h @@ -164,4 +164,15 @@ struct platform_device; int acpi_create_platform_device(struct acpi_device *adev, const struct acpi_device_id *id);
+/*-------------------------------------------------------------------------- + Video + -------------------------------------------------------------------------- */ +#if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) +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_ */
On Wed, Jul 17, 2013 at 7:16 PM, Rafael J. Wysocki rjw@sisk.pl wrote:
On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote:
Windows 8 introduced new policy for backlight control by pushing it out to graphics drivers. This appears to have coincided with a range of vendors adding Windows 8 checks to their backlight control code which trigger either awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest thing to do would be to just disable ACPI backlight control entirely if the firmware indicates Windows 8 support, but it's entirely possible that individual graphics drivers might still make use of the ACPI functionality in preference to native control.
The first two patches in this series are picked from other patchesets aimed at solving similar problems. The last simply unregisters ACPI backlight control on Windows 8 systems when using an Intel GPU. Similar code could be added to other drivers, but I'm reluctant to do so without further investigation as to the behaviour of the vendor drivers under Windows.
Well, after some more time spent on that, we now have a series of 3 patches (different from the $subject one) that we think may be used to address this issue. As far as I can say, it has been tested by multiple people whose systems have those problems and they generally saw improvement.
It is not my ideal approach, but it seems to be the least intrusive and/or with the least amount of possible side effects that we can do right now as a general measure (alternatively, we could create a possibly long blacklist table of affected systems with different workarounds for them, but let's just say that is not overwhelmingly attractive).
[1/3] Make ACPICA export things that we need for checking OSI(Win8).
[2/3] Make acpi_video_device_find_cap() call acpi_video_init_brightness() even if it is not going to register the backlight interface (needed for Thinkpads).
[3/3] Avoid using ACPI backlight if i915 is in use and the firmware believes we are Windows 8.
Many thanks to everyone involved!
I tried this patch series and it's as I expected, it's the same as acpi_backlight=vendor, and the intel backlight driver doesn't work correctly in this machine. If you are actually serious about the mantra of "no user-space regressions", then for this machine at least, you need to use the ACPI backlight with Windows8 OSI disabled, until the intel backlight driver is fixed. My patch does that:
http://article.gmane.org/gmane.linux.acpi.devel/60969
On Thursday, July 18, 2013 02:16:09 AM Rafael J. Wysocki wrote:
On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote:
Windows 8 introduced new policy for backlight control by pushing it out to graphics drivers. This appears to have coincided with a range of vendors adding Windows 8 checks to their backlight control code which trigger either awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest thing to do would be to just disable ACPI backlight control entirely if the firmware indicates Windows 8 support, but it's entirely possible that individual graphics drivers might still make use of the ACPI functionality in preference to native control.
The first two patches in this series are picked from other patchesets aimed at solving similar problems. The last simply unregisters ACPI backlight control on Windows 8 systems when using an Intel GPU. Similar code could be added to other drivers, but I'm reluctant to do so without further investigation as to the behaviour of the vendor drivers under Windows.
Well, after some more time spent on that, we now have a series of 3 patches (different from the $subject one) that we think may be used to address this issue. As far as I can say, it has been tested by multiple people whose systems have those problems and they generally saw improvement.
It is not my ideal approach, but it seems to be the least intrusive and/or with the least amount of possible side effects that we can do right now as a general measure (alternatively, we could create a possibly long blacklist table of affected systems with different workarounds for them, but let's just say that is not overwhelmingly attractive).
[1/3] Make ACPICA export things that we need for checking OSI(Win8).
[2/3] Make acpi_video_device_find_cap() call acpi_video_init_brightness() even if it is not going to register the backlight interface (needed for Thinkpads).
[3/3] Avoid using ACPI backlight if i915 is in use and the firmware believes we are Windows 8.
Many thanks to everyone involved!
So this didn't work, as we had to revert [3/3], but I think we should try to make some progress with this nevertheless. A way forward I'm seeing now could be to (1) Split the ACPI video driver so that it is possible to register the backlight control separately from the event interface. (2) Add a command line option to i915 to make it use the native backlight control (without registering the ACPI one) if set. Make unset the default initially. (3) Fix i915 backlight control issues for all systems known to have them (that may take a while) and flip the defailt for that option to set when we think we're ready. (4) If there still are problem reports, flip the default back to unset and repeat (3).
If this converges, everyone will be using the native backlight control by default and the original problem will go away automatically.
Thoughts?
Rafael
On Wed, 2013-07-31 at 02:01 +0200, Rafael J. Wysocki wrote:
(3) Fix i915 backlight control issues for all systems known to have them (that may take a while) and flip the defailt for that option to set when we think we're ready.
Unfortunately I don't have any systems that reproduce problems here, so I think Intel are going to have to take the lead on this one.
On Wed, 2013-07-31 at 02:01 +0200, Rafael J. Wysocki wrote:
On Thursday, July 18, 2013 02:16:09 AM Rafael J. Wysocki wrote:
On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote:
Windows 8 introduced new policy for backlight control by pushing it out to graphics drivers. This appears to have coincided with a range of vendors adding Windows 8 checks to their backlight control code which trigger either awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest thing to do would be to just disable ACPI backlight control entirely if the firmware indicates Windows 8 support, but it's entirely possible that individual graphics drivers might still make use of the ACPI functionality in preference to native control.
The first two patches in this series are picked from other patchesets aimed at solving similar problems. The last simply unregisters ACPI backlight control on Windows 8 systems when using an Intel GPU. Similar code could be added to other drivers, but I'm reluctant to do so without further investigation as to the behaviour of the vendor drivers under Windows.
Well, after some more time spent on that, we now have a series of 3 patches (different from the $subject one) that we think may be used to address this issue. As far as I can say, it has been tested by multiple people whose systems have those problems and they generally saw improvement.
It is not my ideal approach, but it seems to be the least intrusive and/or with the least amount of possible side effects that we can do right now as a general measure (alternatively, we could create a possibly long blacklist table of affected systems with different workarounds for them, but let's just say that is not overwhelmingly attractive).
[1/3] Make ACPICA export things that we need for checking OSI(Win8).
[2/3] Make acpi_video_device_find_cap() call acpi_video_init_brightness() even if it is not going to register the backlight interface (needed for Thinkpads).
[3/3] Avoid using ACPI backlight if i915 is in use and the firmware believes we are Windows 8.
Many thanks to everyone involved!
So this didn't work, as we had to revert [3/3], but I think we should try to make some progress with this nevertheless. A way forward I'm seeing now could be to (1) Split the ACPI video driver so that it is possible to register the backlight control separately from the event interface. (2) Add a command line option to i915 to make it use the native backlight control (without registering the ACPI one) if set. Make unset the default initially. (3) Fix i915 backlight control issues for all systems known to have them (that may take a while) and flip the defailt for that option to set when we think we're ready. (4) If there still are problem reports, flip the default back to unset and repeat (3).
If this converges, everyone will be using the native backlight control by default and the original problem will go away automatically.
Thoughts?
Rafael
Good idea. I have 3-4 laptops with this problem. I can test, but I don't have devices, on which found regressions (in backlight) in previous patch.
On 07/31/2013 08:01 AM, Rafael J. Wysocki wrote:
On Thursday, July 18, 2013 02:16:09 AM Rafael J. Wysocki wrote:
On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote:
Windows 8 introduced new policy for backlight control by pushing it out to graphics drivers. This appears to have coincided with a range of vendors adding Windows 8 checks to their backlight control code which trigger either awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest thing to do would be to just disable ACPI backlight control entirely if the firmware indicates Windows 8 support, but it's entirely possible that individual graphics drivers might still make use of the ACPI functionality in preference to native control.
The first two patches in this series are picked from other patchesets aimed at solving similar problems. The last simply unregisters ACPI backlight control on Windows 8 systems when using an Intel GPU. Similar code could be added to other drivers, but I'm reluctant to do so without further investigation as to the behaviour of the vendor drivers under Windows.
Well, after some more time spent on that, we now have a series of 3 patches (different from the $subject one) that we think may be used to address this issue. As far as I can say, it has been tested by multiple people whose systems have those problems and they generally saw improvement.
It is not my ideal approach, but it seems to be the least intrusive and/or with the least amount of possible side effects that we can do right now as a general measure (alternatively, we could create a possibly long blacklist table of affected systems with different workarounds for them, but let's just say that is not overwhelmingly attractive).
[1/3] Make ACPICA export things that we need for checking OSI(Win8).
[2/3] Make acpi_video_device_find_cap() call acpi_video_init_brightness() even if it is not going to register the backlight interface (needed for Thinkpads).
[3/3] Avoid using ACPI backlight if i915 is in use and the firmware believes we are Windows 8.
Many thanks to everyone involved!
So this didn't work, as we had to revert [3/3], but I think we should try to make some progress with this nevertheless. A way forward I'm seeing now could be to (1) Split the ACPI video driver so that it is possible to register the backlight control separately from the event interface. (2) Add a command line option to i915 to make it use the native backlight control (without registering the ACPI one) if set. Make unset the default initially. (3) Fix i915 backlight control issues for all systems known to have them (that may take a while) and flip the defailt for that option to set when we think we're ready. (4) If there still are problem reports, flip the default back to unset and repeat (3).
If this converges, everyone will be using the native backlight control by default and the original problem will go away automatically.
Thoughts?
Sounds good to me.
-Aaron
On Sun, Jun 09, 2013 at 07:01:36PM -0400, Matthew Garrett wrote:
Windows 8 introduced new policy for backlight control by pushing it out to graphics drivers. This appears to have coincided with a range of vendors adding Windows 8 checks to their backlight control code which trigger either awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest thing to do would be to just disable ACPI backlight control entirely if the firmware indicates Windows 8 support, but it's entirely possible that individual graphics drivers might still make use of the ACPI functionality in preference to native control.
Maybe tangential, so Aaron and I were wondering on https://bugzilla.kernel.org/show_bug.cgi?id=60680 whether it would make sense to handle the backlight control strictly in the kernel, without going to userspace and back?
Background is that on my x230, I needed to connect the Fn-Fx backlight hotkey presses to a script to write to /sys/class/backlight/intel_backlight/brightness because Fluxbox doesn't do that (and maybe doesn't have to).
So, without presuming any ACPI or backlight knowledge, can we make the backlight control work only in the kernel by connecting the hotkey presses to some backlight controlling interface which backlight-capable devices implement so that it works regardless of userspace environment? Even if the machine is not running X?
Hmmm.
Thanks.
On 08/07/2013 03:44 PM, Borislav Petkov wrote:
On Sun, Jun 09, 2013 at 07:01:36PM -0400, Matthew Garrett wrote:
Windows 8 introduced new policy for backlight control by pushing it out to graphics drivers. This appears to have coincided with a range of vendors adding Windows 8 checks to their backlight control code which trigger either awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest thing to do would be to just disable ACPI backlight control entirely if the firmware indicates Windows 8 support, but it's entirely possible that individual graphics drivers might still make use of the ACPI functionality in preference to native control.
Maybe tangential, so Aaron and I were wondering on https://bugzilla.kernel.org/show_bug.cgi?id=60680 whether it would make sense to handle the backlight control strictly in the kernel, without going to userspace and back?
I think this would require the kernel has the knowledge of which backlight interface this system is using or should be using, or it wouldn't know which interface should receive and process the event...
-Aaron
Background is that on my x230, I needed to connect the Fn-Fx backlight hotkey presses to a script to write to /sys/class/backlight/intel_backlight/brightness because Fluxbox doesn't do that (and maybe doesn't have to).
So, without presuming any ACPI or backlight knowledge, can we make the backlight control work only in the kernel by connecting the hotkey presses to some backlight controlling interface which backlight-capable devices implement so that it works regardless of userspace environment? Even if the machine is not running X?
Hmmm.
Thanks.
On Wed, Aug 07, 2013 at 05:03:15PM +0800, Aaron Lu wrote:
I think this would require the kernel has the knowledge of which backlight interface this system is using or should be using, or it wouldn't know which interface should receive and process the event...
Well, we can have a default one, say acpi_video and make it overridable with command line options like acpi_backlight=vendor or acpi_backlight=gpu, for example, if acpi_video suffers of a major suckage...
On Wed, 2013-08-07 at 09:44 +0200, Borislav Petkov wrote:
Background is that on my x230, I needed to connect the Fn-Fx backlight hotkey presses to a script to write to /sys/class/backlight/intel_backlight/brightness because Fluxbox doesn't do that (and maybe doesn't have to).
So, without presuming any ACPI or backlight knowledge, can we make the backlight control work only in the kernel by connecting the hotkey presses to some backlight controlling interface which backlight-capable devices implement so that it works regardless of userspace environment? Even if the machine is not running X?
Not really. The ACPI driver is able to do this because the events and the backlight are associated with the same device. We could potentially make something work with GPU backlight drivers since their PCI device should also be associated with the backlight device, but things get complicated quickly - once ddcci support is hooked up you'll also have kernel backlight devices for some external monitors, and now you need to make a policy decision about which display should be controlled in response to the keypress. One reasonable choice would be whichever display contains the currently focused window, which is obviously out of scope for the kernel.
So if we're going to do this then we need a generalised mechanism in-kernel for connecting input devices to backlight devices, and we need a mechanism for disabling it when userspace knows better. This sounds like a lot of work for something that should really just be handled by userspace already.
On Wed, Aug 07, 2013 at 10:36:08AM +0000, Matthew Garrett wrote:
Not really. The ACPI driver is able to do this because the events and the backlight are associated with the same device.
Well, it doesn't work at least in my case.
I need to echo stuff into /sys/class/backlight/intel_backlight :(
We could potentially make something work with GPU backlight drivers since their PCI device should also be associated with the backlight device, but things get complicated quickly - once ddcci support is hooked up you'll also have kernel backlight devices for some external monitors, and now you need to make a policy decision about which display should be controlled in response to the keypress. One reasonable choice would be whichever display contains the currently focused window, which is obviously out of scope for the kernel.
So if we're going to do this then we need a generalised mechanism in-kernel for connecting input devices to backlight devices, and we need a mechanism for disabling it when userspace knows better. This sounds like a lot of work for something that should really just be handled by userspace already.
That's easy: single-monitor setups have it enabled by default. So every laptop out there will just work, without any userspace intervention.
More complicated situations are presented with the opportunity to disable the kernel-side control and do it themselves.
dri-devel@lists.freedesktop.org
-
Aaron Lu -
Aaron Lu -
Alex Deucher -
Borislav Petkov -
Daniel Vetter -
Daniel Vetter -
Felipe Contreras -
Igor Gnatenko -
joeyli -
Matthew Garrett -
Matthew Garrett -
Rafael J. Wysocki -
Seth Forshee -
Yves-Alexis Perez