The ACPI video module provides two functionalities: backlight control and backlight hotkey event delivery. It is possible the backlight control interface is broken while the system still needs its event delivery, so it's worth to seperate the two interfaces clearly.
This patchset has two patches, the first is to sepeate the two interfaces in the ACPI video module and the second one solves some Win8 backlight control problems by removing ACPI video's backlight interface while still keeping its event delivery functionality. Due to some systems whose firmware claims win8 compatible have problems with i915's backlight control interface, a module param is introduced to give user a chance to select if they want to remove ACPI video's backlight control interface. The param is set to false by default.
Aaron Lu (2): ACPI / video: seperate backlight control and event interface ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8
drivers/acpi/internal.h | 2 + drivers/acpi/video.c | 473 +++++++++++++++++++++++----------------- drivers/acpi/video_detect.c | 15 +- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 5 + drivers/gpu/drm/i915/i915_drv.h | 1 + include/acpi/video.h | 11 +- include/linux/acpi.h | 1 + 8 files changed, 310 insertions(+), 200 deletions(-)
The backlight control and event delivery functionality provided by ACPI video module is mixed together and registered all during video device enumeration time. As a result, the two functionality are also removed together on module unload time or by the acpi_video_unregister function. The two functionalities are actually independent and one may be useful while the other one may be broken, so it is desirable to seperate the two functionalities such that it is clear and easy to disable one functionality without affecting the other one. This patch does the seperation and as a result, a new video_bus_head list is introduced to store all registered video bus structure and a new function acpi_video_unregister_backlight is introduced to unregister backlight interfaces for all video devices belonging to stored video buses.
Currently, there is no need to unregister ACPI video's event delivery functionality alone so the function acpi_video_remove_notify_handler is not introduced, it can be easily added when needed.
Signed-off-by: Aaron Lu aaron.lu@intel.com --- drivers/acpi/video.c | 451 ++++++++++++++++++++++++++++++--------------------- include/acpi/video.h | 2 + 2 files changed, 264 insertions(+), 189 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index aebcf63..5ad5a71 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -89,6 +89,8 @@ static bool use_bios_initial_backlight = 1; module_param(use_bios_initial_backlight, bool, 0644);
static int register_count; +static struct mutex video_list_lock; +static struct list_head video_bus_head; static int acpi_video_bus_add(struct acpi_device *device); static int acpi_video_bus_remove(struct acpi_device *device); static void acpi_video_bus_notify(struct acpi_device *device, u32 event); @@ -157,6 +159,7 @@ struct acpi_video_bus { struct acpi_video_bus_flags flags; struct list_head video_device_list; struct mutex device_list_lock; /* protects video_device_list */ + struct list_head entry; struct input_dev *input; char phys[32]; /* for input device */ struct notifier_block pm_nb; @@ -884,79 +887,6 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
if (acpi_has_method(device->dev->handle, "_DDC")) device->cap._DDC = 1; - - if (acpi_video_backlight_support()) { - struct backlight_properties props; - struct pci_dev *pdev; - acpi_handle acpi_parent; - struct device *parent = NULL; - int result; - static int count; - char *name; - - result = acpi_video_init_brightness(device); - if (result) - return; - name = kasprintf(GFP_KERNEL, "acpi_video%d", count); - if (!name) - return; - count++; - - acpi_get_parent(device->dev->handle, &acpi_parent); - - pdev = acpi_get_pci_dev(acpi_parent); - if (pdev) { - parent = &pdev->dev; - pci_dev_put(pdev); - } - - memset(&props, 0, sizeof(struct backlight_properties)); - props.type = BACKLIGHT_FIRMWARE; - props.max_brightness = device->brightness->count - 3; - device->backlight = backlight_device_register(name, - parent, - device, - &acpi_backlight_ops, - &props); - kfree(name); - if (IS_ERR(device->backlight)) - return; - - /* - * Save current brightness level in case we have to restore it - * before acpi_video_device_lcd_set_level() is called next time. - */ - device->backlight->props.brightness = - acpi_video_get_brightness(device->backlight); - - device->cooling_dev = thermal_cooling_device_register("LCD", - device->dev, &video_cooling_ops); - if (IS_ERR(device->cooling_dev)) { - /* - * Set cooling_dev to NULL so we don't crash trying to - * free it. - * Also, why the hell we are returning early and - * not attempt to register video output if cooling - * device registration failed? - * -- dtor - */ - device->cooling_dev = NULL; - return; - } - - dev_info(&device->dev->dev, "registered as cooling_device%d\n", - device->cooling_dev->id); - result = sysfs_create_link(&device->dev->dev.kobj, - &device->cooling_dev->device.kobj, - "thermal_cooling"); - if (result) - printk(KERN_ERR PREFIX "Create sysfs link\n"); - result = sysfs_create_link(&device->cooling_dev->device.kobj, - &device->dev->dev.kobj, "device"); - if (result) - printk(KERN_ERR PREFIX "Create sysfs link\n"); - - } }
/* @@ -1143,13 +1073,6 @@ acpi_video_bus_get_one_device(struct acpi_device *device, acpi_video_device_bind(video, data); acpi_video_device_find_cap(data);
- status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, - acpi_video_device_notify, data); - if (ACPI_FAILURE(status)) - dev_err(&device->dev, "Error installing notify handler\n"); - else - data->flags.notify = 1; - mutex_lock(&video->device_list_lock); list_add_tail(&data->entry, &video->video_device_list); mutex_unlock(&video->device_list_lock); @@ -1454,64 +1377,6 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video, return status; }
-static int acpi_video_bus_put_one_device(struct acpi_video_device *device) -{ - acpi_status status; - - if (!device || !device->video) - return -ENOENT; - - if (device->flags.notify) { - status = acpi_remove_notify_handler(device->dev->handle, - ACPI_DEVICE_NOTIFY, acpi_video_device_notify); - if (ACPI_FAILURE(status)) - dev_err(&device->dev->dev, - "Can't remove video notify handler\n"); - } - - if (device->backlight) { - backlight_device_unregister(device->backlight); - device->backlight = NULL; - } - if (device->cooling_dev) { - sysfs_remove_link(&device->dev->dev.kobj, - "thermal_cooling"); - sysfs_remove_link(&device->cooling_dev->device.kobj, - "device"); - thermal_cooling_device_unregister(device->cooling_dev); - device->cooling_dev = NULL; - } - - return 0; -} - -static int acpi_video_bus_put_devices(struct acpi_video_bus *video) -{ - int status; - struct acpi_video_device *dev, *next; - - mutex_lock(&video->device_list_lock); - - list_for_each_entry_safe(dev, next, &video->video_device_list, entry) { - - status = acpi_video_bus_put_one_device(dev); - if (ACPI_FAILURE(status)) - printk(KERN_WARNING PREFIX - "hhuuhhuu bug in acpi video driver.\n"); - - if (dev->brightness) { - kfree(dev->brightness->levels); - kfree(dev->brightness); - } - list_del(&dev->entry); - kfree(dev); - } - - mutex_unlock(&video->device_list_lock); - - return 0; -} - /* acpi_video interface */
/* @@ -1536,7 +1401,7 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event) struct input_dev *input; int keycode = 0;
- if (!video) + if (!video || !video->input) return;
input = video->input; @@ -1691,12 +1556,253 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context, return AE_OK; }
+static void acpi_video_dev_register_backlight(struct acpi_video_device *device) +{ + if (acpi_video_backlight_support()) { + struct backlight_properties props; + struct pci_dev *pdev; + acpi_handle acpi_parent; + struct device *parent = NULL; + int result; + static int count; + char *name; + + result = acpi_video_init_brightness(device); + if (result) + return; + name = kasprintf(GFP_KERNEL, "acpi_video%d", count); + if (!name) + return; + count++; + + acpi_get_parent(device->dev->handle, &acpi_parent); + + pdev = acpi_get_pci_dev(acpi_parent); + if (pdev) { + parent = &pdev->dev; + pci_dev_put(pdev); + } + + memset(&props, 0, sizeof(struct backlight_properties)); + props.type = BACKLIGHT_FIRMWARE; + props.max_brightness = device->brightness->count - 3; + device->backlight = backlight_device_register(name, + parent, + device, + &acpi_backlight_ops, + &props); + kfree(name); + if (IS_ERR(device->backlight)) + return; + + /* + * Save current brightness level in case we have to restore it + * before acpi_video_device_lcd_set_level() is called next time. + */ + device->backlight->props.brightness = + acpi_video_get_brightness(device->backlight); + + device->cooling_dev = thermal_cooling_device_register("LCD", + device->dev, &video_cooling_ops); + if (IS_ERR(device->cooling_dev)) { + /* + * Set cooling_dev to NULL so we don't crash trying to + * free it. + * Also, why the hell we are returning early and + * not attempt to register video output if cooling + * device registration failed? + * -- dtor + */ + device->cooling_dev = NULL; + return; + } + + dev_info(&device->dev->dev, "registered as cooling_device%d\n", + device->cooling_dev->id); + result = sysfs_create_link(&device->dev->dev.kobj, + &device->cooling_dev->device.kobj, + "thermal_cooling"); + if (result) + printk(KERN_ERR PREFIX "Create sysfs link\n"); + result = sysfs_create_link(&device->cooling_dev->device.kobj, + &device->dev->dev.kobj, "device"); + if (result) + printk(KERN_ERR PREFIX "Create sysfs link\n"); + } +} + +static int acpi_video_bus_register_backlight(struct acpi_video_bus *video) +{ + struct acpi_video_device *dev; + + mutex_lock(&video->device_list_lock); + list_for_each_entry(dev, &video->video_device_list, entry) + acpi_video_dev_register_backlight(dev); + mutex_unlock(&video->device_list_lock); + + video->pm_nb.notifier_call = acpi_video_resume; + video->pm_nb.priority = 0; + return register_pm_notifier(&video->pm_nb); +} + +static void acpi_video_dev_unregister_backlight(struct acpi_video_device *device) +{ + if (device->backlight) { + backlight_device_unregister(device->backlight); + device->backlight = NULL; + } + if (device->brightness) { + kfree(device->brightness->levels); + kfree(device->brightness); + device->brightness = NULL; + } + if (device->cooling_dev) { + sysfs_remove_link(&device->dev->dev.kobj, "thermal_cooling"); + sysfs_remove_link(&device->cooling_dev->device.kobj, "device"); + thermal_cooling_device_unregister(device->cooling_dev); + device->cooling_dev = NULL; + } +} + +static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video) +{ + struct acpi_video_device *dev; + int error = unregister_pm_notifier(&video->pm_nb); + + mutex_lock(&video->device_list_lock); + list_for_each_entry(dev, &video->video_device_list, entry) + acpi_video_dev_unregister_backlight(dev); + mutex_unlock(&video->device_list_lock); + + return error; +} + +int acpi_video_unregister_backlight(void) +{ + struct acpi_video_bus *video; + int error = 0; + + mutex_lock(&video_list_lock); + list_for_each_entry(video, &video_bus_head, entry) { + error = acpi_video_bus_unregister_backlight(video); + if (error) + break; + } + mutex_unlock(&video_list_lock); + + return error; +} +EXPORT_SYMBOL(acpi_video_unregister_backlight); + +static void acpi_video_dev_add_notify_handler(struct acpi_video_device *device) +{ + acpi_status status; + struct acpi_device *adev = device->dev; + + status = acpi_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, + acpi_video_device_notify, device); + if (ACPI_FAILURE(status)) + dev_err(&adev->dev, "Error installing notify handler\n"); + else + device->flags.notify = 1; +} + +static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video) +{ + struct input_dev *input; + struct acpi_video_device *dev; + int error; + + video->input = input = input_allocate_device(); + if (!input) { + error = -ENOMEM; + goto out; + } + + error = acpi_video_bus_start_devices(video); + if (error) + goto err_free_input; + + snprintf(video->phys, sizeof(video->phys), + "%s/video/input0", acpi_device_hid(video->device)); + + input->name = acpi_device_name(video->device); + input->phys = video->phys; + input->id.bustype = BUS_HOST; + input->id.product = 0x06; + input->dev.parent = &video->device->dev; + input->evbit[0] = BIT(EV_KEY); + set_bit(KEY_SWITCHVIDEOMODE, input->keybit); + set_bit(KEY_VIDEO_NEXT, input->keybit); + set_bit(KEY_VIDEO_PREV, input->keybit); + set_bit(KEY_BRIGHTNESS_CYCLE, input->keybit); + set_bit(KEY_BRIGHTNESSUP, input->keybit); + set_bit(KEY_BRIGHTNESSDOWN, input->keybit); + set_bit(KEY_BRIGHTNESS_ZERO, input->keybit); + set_bit(KEY_DISPLAY_OFF, input->keybit); + + error = input_register_device(input); + if (error) + goto err_stop_dev; + + mutex_lock(&video->device_list_lock); + list_for_each_entry(dev, &video->video_device_list, entry) + acpi_video_dev_add_notify_handler(dev); + mutex_unlock(&video->device_list_lock); + + return 0; + +err_stop_dev: + acpi_video_bus_stop_devices(video); +err_free_input: + input_free_device(input); + video->input = NULL; +out: + return error; +} + +static void acpi_video_dev_remove_notify_handler(struct acpi_video_device *dev) +{ + if (dev->flags.notify) { + acpi_remove_notify_handler(dev->dev->handle, ACPI_DEVICE_NOTIFY, + acpi_video_device_notify); + dev->flags.notify = 0; + } +} + +static void acpi_video_bus_remove_notify_handler(struct acpi_video_bus *video) +{ + struct acpi_video_device *dev; + + mutex_lock(&video->device_list_lock); + list_for_each_entry(dev, &video->video_device_list, entry) + acpi_video_dev_remove_notify_handler(dev); + mutex_unlock(&video->device_list_lock); + + acpi_video_bus_stop_devices(video); + input_unregister_device(video->input); + video->input = NULL; +} + +static int acpi_video_bus_put_devices(struct acpi_video_bus *video) +{ + struct acpi_video_device *dev, *next; + + mutex_lock(&video->device_list_lock); + list_for_each_entry_safe(dev, next, &video->video_device_list, entry) { + list_del(&dev->entry); + kfree(dev); + } + mutex_unlock(&video->device_list_lock); + + return 0; +} + static int instance;
static int acpi_video_bus_add(struct acpi_device *device) { struct acpi_video_bus *video; - struct input_dev *input; int error; acpi_status status;
@@ -1748,62 +1854,24 @@ static int acpi_video_bus_add(struct acpi_device *device) if (error) goto err_put_video;
- video->input = input = input_allocate_device(); - if (!input) { - error = -ENOMEM; - goto err_put_video; - } - - error = acpi_video_bus_start_devices(video); - if (error) - goto err_free_input_dev; - - snprintf(video->phys, sizeof(video->phys), - "%s/video/input0", acpi_device_hid(video->device)); - - input->name = acpi_device_name(video->device); - input->phys = video->phys; - input->id.bustype = BUS_HOST; - input->id.product = 0x06; - input->dev.parent = &device->dev; - input->evbit[0] = BIT(EV_KEY); - set_bit(KEY_SWITCHVIDEOMODE, input->keybit); - set_bit(KEY_VIDEO_NEXT, input->keybit); - set_bit(KEY_VIDEO_PREV, input->keybit); - set_bit(KEY_BRIGHTNESS_CYCLE, input->keybit); - set_bit(KEY_BRIGHTNESSUP, input->keybit); - set_bit(KEY_BRIGHTNESSDOWN, input->keybit); - set_bit(KEY_BRIGHTNESS_ZERO, input->keybit); - set_bit(KEY_DISPLAY_OFF, input->keybit); - printk(KERN_INFO PREFIX "%s [%s] (multi-head: %s rom: %s post: %s)\n", ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device), video->flags.multihead ? "yes" : "no", video->flags.rom ? "yes" : "no", video->flags.post ? "yes" : "no"); + mutex_lock(&video_list_lock); + list_add_tail(&video->entry, &video_bus_head); + mutex_unlock(&video_list_lock);
- video->pm_nb.notifier_call = acpi_video_resume; - video->pm_nb.priority = 0; - error = register_pm_notifier(&video->pm_nb); - if (error) - goto err_stop_video; - - error = input_register_device(input); - if (error) - goto err_unregister_pm_notifier; + acpi_video_bus_register_backlight(video); + acpi_video_bus_add_notify_handler(video);
return 0;
- err_unregister_pm_notifier: - unregister_pm_notifier(&video->pm_nb); - err_stop_video: - acpi_video_bus_stop_devices(video); - err_free_input_dev: - input_free_device(input); - err_put_video: +err_put_video: acpi_video_bus_put_devices(video); kfree(video->attached_array); - err_free_video: +err_free_video: kfree(video); device->driver_data = NULL;
@@ -1820,12 +1888,14 @@ static int acpi_video_bus_remove(struct acpi_device *device)
video = acpi_driver_data(device);
- unregister_pm_notifier(&video->pm_nb); - - acpi_video_bus_stop_devices(video); + acpi_video_bus_remove_notify_handler(video); + acpi_video_bus_unregister_backlight(video); acpi_video_bus_put_devices(video);
- input_unregister_device(video->input); + mutex_lock(&video_list_lock); + list_del(&video->entry); + mutex_unlock(&video_list_lock); + kfree(video->attached_array); kfree(video);
@@ -1874,6 +1944,9 @@ int acpi_video_register(void) return 0; }
+ mutex_init(&video_list_lock); + INIT_LIST_HEAD(&video_bus_head); + result = acpi_bus_register_driver(&acpi_video_bus); if (result < 0) return -ENODEV; diff --git a/include/acpi/video.h b/include/acpi/video.h index 61109f2..0c665b5 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 int acpi_video_unregister_backlight(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 int acpi_video_unregister_backlight(void) { return; } static inline int acpi_video_get_edid(struct acpi_device *device, int type, int device_id, void **edid) {
On Mon, 2013-09-09 at 16:40 +0800, Aaron Lu wrote:
The backlight control and event delivery functionality provided by ACPI video module is mixed together and registered all during video device enumeration time. As a result, the two functionality are also removed together on module unload time or by the acpi_video_unregister function. The two functionalities are actually independent and one may be useful while the other one may be broken, so it is desirable to seperate the two functionalities such that it is clear and easy to disable one functionality without affecting the other one. This patch does the seperation and as a result, a new video_bus_head list is introduced to store all registered video bus structure and a new function acpi_video_unregister_backlight is introduced to unregister backlight interfaces for all video devices belonging to stored video buses.
Currently, there is no need to unregister ACPI video's event delivery functionality alone so the function acpi_video_remove_notify_handler is not introduced, it can be easily added when needed.
Signed-off-by: Aaron Lu aaron.lu@intel.com
Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com
drivers/acpi/video.c | 451 ++++++++++++++++++++++++++++++--------------------- include/acpi/video.h | 2 + 2 files changed, 264 insertions(+), 189 deletions(-)
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(bool), that if ture is passed, 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() instead of acpi_video_register() in i915_driver_load(), and the param passed there is controlled by the i915 module level parameter i915_take_over_backlight, which is set to false by default.
This change is evolved from earlier patches of Matthew Garrett, Chun-Yi Lee and Seth Forshee and is heavily based on two patches from Rafael: https://lkml.org/lkml/2013/7/17/720 https://lkml.org/lkml/2013/7/24/806
Signed-off-by: Aaron Lu aaron.lu@intel.com --- drivers/acpi/internal.h | 2 ++ drivers/acpi/video.c | 24 ++++++++++++++++-------- drivers/acpi/video_detect.c | 15 ++++++++++++++- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 5 +++++ drivers/gpu/drm/i915/i915_drv.h | 1 + include/acpi/video.h | 9 +++++++-- include/linux/acpi.h | 1 + 8 files changed, 47 insertions(+), 12 deletions(-)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 20f4233..a53832e 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev, -------------------------------------------------------------------------- */ #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_ */ diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 5ad5a71..cc709a7 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event) 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) @@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
static void acpi_video_dev_register_backlight(struct acpi_video_device *device) { - if (acpi_video_backlight_support()) { + if (acpi_video_verify_backlight_support()) { struct backlight_properties props; struct pci_dev *pdev; acpi_handle acpi_parent; @@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void) 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_video_unregister_backlight(); + return 0; }
@@ -1959,7 +1967,7 @@ int acpi_video_register(void)
return 0; } -EXPORT_SYMBOL(acpi_video_register); +EXPORT_SYMBOL(__acpi_video_register);
void acpi_video_unregister(void) { diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 940edbf..c7e43ec 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void)
bool acpi_video_backlight_quirks(void) { - return acpi_gbl_osi_data >= ACPI_OSI_WIN_8; + 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);
@@ -283,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. diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f466980..75fba17 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (INTEL_INFO(dev)->num_pipes) { /* Must be done after probing outputs */ intel_opregion_init(dev); - acpi_video_register(); + __acpi_video_register(i915_take_over_backlight); }
if (IS_GEN5(dev)) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 45b3c03..b014398 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -132,6 +132,11 @@ int i915_enable_ips __read_mostly = 1; module_param_named(enable_ips, i915_enable_ips, int, 0600); MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
+bool i915_take_over_backlight = false; +module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644); +MODULE_PARM_DESC(take_over_backlight, + "Prevent ACPI backlight from being used (default: false)"); + static struct drm_driver driver; extern int intel_agp_enabled;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1929bff..b43c430 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly; extern unsigned int i915_preliminary_hw_support __read_mostly; extern int i915_disable_power_well __read_mostly; extern int i915_enable_ips __read_mostly; +extern bool i915_take_over_backlight __read_mostly;
extern int i915_suspend(struct drm_device *dev, pm_message_t state); extern int i915_resume(struct drm_device *dev); diff --git a/include/acpi/video.h b/include/acpi/video.h index 0c665b5..dc5b8ad 100644 --- a/include/acpi/video.h +++ b/include/acpi/video.h @@ -17,13 +17,13 @@ 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); extern void acpi_video_unregister(void); extern int acpi_video_unregister_backlight(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(bool backlight_quirks) { return 0; } static inline void acpi_video_unregister(void) { return; } static inline int acpi_video_unregister_backlight(void) { return; } static inline int acpi_video_get_edid(struct acpi_device *device, int type, @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type, } #endif
+static inline int acpi_video_register(void) +{ + return __acpi_video_register(false); +} + #endif diff --git a/include/linux/acpi.h b/include/linux/acpi.h index a5db4ae..a0dcce5 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid); #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)
Hi Aaaron,
Have we grown any clue meanwhile about which Intel boxes need this and for which we still need to keep the acpi backlight around? I've grown _very_ reluctant to just adding tons of quirks to our driver for the backlight. Almost all the quirks we have added recently (or that have been proposed to be added) are for the backlight. Imo that indicates we get something fundamentally wrong ...
Cheers, Daniel
On Mon, Sep 09, 2013 at 04:42:20PM +0800, Aaron Lu wrote:
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(bool), that if ture is passed, 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() instead of acpi_video_register() in i915_driver_load(), and the param passed there is controlled by the i915 module level parameter i915_take_over_backlight, which is set to false by default.
This change is evolved from earlier patches of Matthew Garrett, Chun-Yi Lee and Seth Forshee and is heavily based on two patches from Rafael: https://lkml.org/lkml/2013/7/17/720 https://lkml.org/lkml/2013/7/24/806
Signed-off-by: Aaron Lu aaron.lu@intel.com
drivers/acpi/internal.h | 2 ++ drivers/acpi/video.c | 24 ++++++++++++++++-------- drivers/acpi/video_detect.c | 15 ++++++++++++++- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 5 +++++ drivers/gpu/drm/i915/i915_drv.h | 1 + include/acpi/video.h | 9 +++++++-- include/linux/acpi.h | 1 + 8 files changed, 47 insertions(+), 12 deletions(-)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 20f4233..a53832e 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev, -------------------------------------------------------------------------- */ #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_ */ diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 5ad5a71..cc709a7 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event) 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)
@@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
static void acpi_video_dev_register_backlight(struct acpi_video_device *device) {
- if (acpi_video_backlight_support()) {
- if (acpi_video_verify_backlight_support()) { struct backlight_properties props; struct pci_dev *pdev; acpi_handle acpi_parent;
@@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void) 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_video_unregister_backlight();- return 0; }
@@ -1959,7 +1967,7 @@ int acpi_video_register(void)
return 0; } -EXPORT_SYMBOL(acpi_video_register); +EXPORT_SYMBOL(__acpi_video_register);
void acpi_video_unregister(void) { diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 940edbf..c7e43ec 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void)
bool acpi_video_backlight_quirks(void) {
- return acpi_gbl_osi_data >= ACPI_OSI_WIN_8;
- 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);
@@ -283,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.
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f466980..75fba17 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (INTEL_INFO(dev)->num_pipes) { /* Must be done after probing outputs */ intel_opregion_init(dev);
acpi_video_register();
__acpi_video_register(i915_take_over_backlight);}
if (IS_GEN5(dev))
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 45b3c03..b014398 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -132,6 +132,11 @@ int i915_enable_ips __read_mostly = 1; module_param_named(enable_ips, i915_enable_ips, int, 0600); MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
+bool i915_take_over_backlight = false; +module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644); +MODULE_PARM_DESC(take_over_backlight,
"Prevent ACPI backlight from being used (default: false)");static struct drm_driver driver; extern int intel_agp_enabled;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1929bff..b43c430 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly; extern unsigned int i915_preliminary_hw_support __read_mostly; extern int i915_disable_power_well __read_mostly; extern int i915_enable_ips __read_mostly; +extern bool i915_take_over_backlight __read_mostly;
extern int i915_suspend(struct drm_device *dev, pm_message_t state); extern int i915_resume(struct drm_device *dev); diff --git a/include/acpi/video.h b/include/acpi/video.h index 0c665b5..dc5b8ad 100644 --- a/include/acpi/video.h +++ b/include/acpi/video.h @@ -17,13 +17,13 @@ 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); extern void acpi_video_unregister(void); extern int acpi_video_unregister_backlight(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(bool backlight_quirks) { return 0; } static inline void acpi_video_unregister(void) { return; } static inline int acpi_video_unregister_backlight(void) { return; } static inline int acpi_video_get_edid(struct acpi_device *device, int type, @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type, } #endif
+static inline int acpi_video_register(void) +{
- return __acpi_video_register(false);
+}
#endif diff --git a/include/linux/acpi.h b/include/linux/acpi.h index a5db4ae..a0dcce5 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid); #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)
-- 1.8.4.12.g2ea3df6
Hi,
On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
Hi Aaaron,
Have we grown any clue meanwhile about which Intel boxes need this and for which we still need to keep the acpi backlight around?
First of all, there is a bunch of boxes where ACPI backlight works incorrectly because of the Win8 compatibility issue. [In short, if we say we are compatible with Win8, the backlight AML goes into a special code path that is broken on those machines. Presumably Win8 uses native backlight control on them and that AML code path is never executed there.] The collection of machines with this problem appears to be kind of random (various models from various vendors), but I *think* they are machines that originally shipped with Win7 with a Win8 "upgrade" option (which in practice requires the BIOS to be updated anyway).
Because of that, last time we tried to switch all of the systems using i915 and telling the BIOS that they are compatible with Win8 over to native backlight control, but that didn't work for two reasons. The first reason is that some user space doesn't know how to use intel_backlight and needs to be taught about that (so some systems are just not ready for that switch). The second and more fundamental reason is that the native backlight control simply doesn't work on some machines and we don't seem to have any idea why and how to debug this particular problem (mostly because we don't have enough information and we don't know what to ask for).
I've grown _very_ reluctant to just adding tons of quirks to our driver for the backlight.
Almost all the quirks we have added recently (or that have been proposed to be added) are for the backlight. Imo that indicates we get something fundamentally wrong ...
This thing isn't really a quirk. It rather is an option for the users of the systems where ACPI backlight doesn't work to switch over to the native backlight control using a command line switch. This way they can at least *see* if the native backlight control works for them and (hopefully) report problems if that's not the case. This gives us a chance to get more information about what the problem is and possibly to make some progress without making changes for everyone, reverting those changes when they don't work etc.
An alternative for them is to pass acpi_osi="!Windows 2012" which will probably make the ACPI backlight work for them again, but this rather is a step back, because we can't possibly learn anything new from that.
Kind regards, Rafael
On Mon, Sep 09, 2013 at 04:42:20PM +0800, Aaron Lu wrote:
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(bool), that if ture is passed, 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() instead of acpi_video_register() in i915_driver_load(), and the param passed there is controlled by the i915 module level parameter i915_take_over_backlight, which is set to false by default.
This change is evolved from earlier patches of Matthew Garrett, Chun-Yi Lee and Seth Forshee and is heavily based on two patches from Rafael: https://lkml.org/lkml/2013/7/17/720 https://lkml.org/lkml/2013/7/24/806
Signed-off-by: Aaron Lu aaron.lu@intel.com
drivers/acpi/internal.h | 2 ++ drivers/acpi/video.c | 24 ++++++++++++++++-------- drivers/acpi/video_detect.c | 15 ++++++++++++++- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 5 +++++ drivers/gpu/drm/i915/i915_drv.h | 1 + include/acpi/video.h | 9 +++++++-- include/linux/acpi.h | 1 + 8 files changed, 47 insertions(+), 12 deletions(-)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 20f4233..a53832e 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev, -------------------------------------------------------------------------- */ #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_ */ diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 5ad5a71..cc709a7 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event) 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)
@@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
static void acpi_video_dev_register_backlight(struct acpi_video_device *device) {
- if (acpi_video_backlight_support()) {
- if (acpi_video_verify_backlight_support()) { struct backlight_properties props; struct pci_dev *pdev; acpi_handle acpi_parent;
@@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void) 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_video_unregister_backlight();- return 0; }
@@ -1959,7 +1967,7 @@ int acpi_video_register(void)
return 0; } -EXPORT_SYMBOL(acpi_video_register); +EXPORT_SYMBOL(__acpi_video_register);
void acpi_video_unregister(void) { diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 940edbf..c7e43ec 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void)
bool acpi_video_backlight_quirks(void) {
- return acpi_gbl_osi_data >= ACPI_OSI_WIN_8;
- 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);
@@ -283,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.
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f466980..75fba17 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (INTEL_INFO(dev)->num_pipes) { /* Must be done after probing outputs */ intel_opregion_init(dev);
acpi_video_register();
__acpi_video_register(i915_take_over_backlight);}
if (IS_GEN5(dev))
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 45b3c03..b014398 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -132,6 +132,11 @@ int i915_enable_ips __read_mostly = 1; module_param_named(enable_ips, i915_enable_ips, int, 0600); MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
+bool i915_take_over_backlight = false; +module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644); +MODULE_PARM_DESC(take_over_backlight,
"Prevent ACPI backlight from being used (default: false)");static struct drm_driver driver; extern int intel_agp_enabled;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1929bff..b43c430 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly; extern unsigned int i915_preliminary_hw_support __read_mostly; extern int i915_disable_power_well __read_mostly; extern int i915_enable_ips __read_mostly; +extern bool i915_take_over_backlight __read_mostly;
extern int i915_suspend(struct drm_device *dev, pm_message_t state); extern int i915_resume(struct drm_device *dev); diff --git a/include/acpi/video.h b/include/acpi/video.h index 0c665b5..dc5b8ad 100644 --- a/include/acpi/video.h +++ b/include/acpi/video.h @@ -17,13 +17,13 @@ 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); extern void acpi_video_unregister(void); extern int acpi_video_unregister_backlight(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(bool backlight_quirks) { return 0; } static inline void acpi_video_unregister(void) { return; } static inline int acpi_video_unregister_backlight(void) { return; } static inline int acpi_video_get_edid(struct acpi_device *device, int type, @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type, } #endif
+static inline int acpi_video_register(void) +{
- return __acpi_video_register(false);
+}
#endif diff --git a/include/linux/acpi.h b/include/linux/acpi.h index a5db4ae..a0dcce5 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid); #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)
On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
Hi,
On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
Hi Aaaron,
Have we grown any clue meanwhile about which Intel boxes need this and for which we still need to keep the acpi backlight around?
First of all, there is a bunch of boxes where ACPI backlight works incorrectly because of the Win8 compatibility issue. [In short, if we say we are compatible with Win8, the backlight AML goes into a special code path that is broken on those machines. Presumably Win8 uses native backlight control on them and that AML code path is never executed there.] The collection of machines with this problem appears to be kind of random (various models from various vendors), but I *think* they are machines that originally shipped with Win7 with a Win8 "upgrade" option (which in practice requires the BIOS to be updated anyway).
Because of that, last time we tried to switch all of the systems using i915 and telling the BIOS that they are compatible with Win8 over to native backlight control, but that didn't work for two reasons. The first reason is that some user space doesn't know how to use intel_backlight and needs to be taught about that (so some systems are just not ready for that switch). The second and more fundamental reason is that the native backlight control simply doesn't work on some machines and we don't seem to have any idea why and how to debug this particular problem (mostly because we don't have enough information and we don't know what to ask for).
I've grown _very_ reluctant to just adding tons of quirks to our driver for the backlight.
Almost all the quirks we have added recently (or that have been proposed to be added) are for the backlight. Imo that indicates we get something fundamentally wrong ...
This thing isn't really a quirk. It rather is an option for the users of the systems where ACPI backlight doesn't work to switch over to the native backlight control using a command line switch. This way they can at least *see* if the native backlight control works for them and (hopefully) report problems if that's not the case. This gives us a chance to get more information about what the problem is and possibly to make some progress without making changes for everyone, reverting those changes when they don't work etc.
An alternative for them is to pass acpi_osi="!Windows 2012" which will probably make the ACPI backlight work for them again, but this rather is a step back, because we can't possibly learn anything new from that.
If Win8 is as broken as we are I'm ok with the module option. It just sounded to me like right now we don't know of a way to make all machines somewhat happy, combined with the other pile of random backlight issues the assumption that we do something (maybe something a bit racy) that windows doesn't do isn't too far-fetched. So I'm not wary of the machines where the aml is busted for acpi_os=win8, but for the others where this broke stuff.
Or do I miss something here?
Cheers, Daniel
On Mon, 2013-09-09 at 17:21 +0200, Daniel Vetter wrote:
If Win8 is as broken as we are I'm ok with the module option. It just sounded to me like right now we don't know of a way to make all machines somewhat happy, combined with the other pile of random backlight issues the assumption that we do something (maybe something a bit racy) that windows doesn't do isn't too far-fetched. So I'm not wary of the machines where the aml is busted for acpi_os=win8, but for the others where this broke stuff.
Windows 8 isn't broken because (as far as we can tell) the Intel drivers under Windows 8 never use the ACPI backlight set function. Of course, it would be nice to have that confirmed by Intel.
On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
Hi,
On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
Hi Aaaron,
Have we grown any clue meanwhile about which Intel boxes need this and for which we still need to keep the acpi backlight around?
First of all, there is a bunch of boxes where ACPI backlight works incorrectly because of the Win8 compatibility issue. [In short, if we say we are compatible with Win8, the backlight AML goes into a special code path that is broken on those machines. Presumably Win8 uses native backlight control on them and that AML code path is never executed there.] The collection of machines with this problem appears to be kind of random (various models from various vendors), but I *think* they are machines that originally shipped with Win7 with a Win8 "upgrade" option (which in practice requires the BIOS to be updated anyway).
Because of that, last time we tried to switch all of the systems using i915 and telling the BIOS that they are compatible with Win8 over to native backlight control, but that didn't work for two reasons. The first reason is that some user space doesn't know how to use intel_backlight and needs to be taught about that (so some systems are just not ready for that switch). The second and more fundamental reason is that the native backlight control simply doesn't work on some machines and we don't seem to have any idea why and how to debug this particular problem (mostly because we don't have enough information and we don't know what to ask for).
I've grown _very_ reluctant to just adding tons of quirks to our driver for the backlight.
Almost all the quirks we have added recently (or that have been proposed to be added) are for the backlight. Imo that indicates we get something fundamentally wrong ...
This thing isn't really a quirk. It rather is an option for the users of the systems where ACPI backlight doesn't work to switch over to the native backlight control using a command line switch. This way they can at least *see* if the native backlight control works for them and (hopefully) report problems if that's not the case. This gives us a chance to get more information about what the problem is and possibly to make some progress without making changes for everyone, reverting those changes when they don't work etc.
An alternative for them is to pass acpi_osi="!Windows 2012" which will probably make the ACPI backlight work for them again, but this rather is a step back, because we can't possibly learn anything new from that.
If Win8 is as broken as we are I'm ok with the module option. It just sounded to me like right now we don't know of a way to make all machines somewhat happy, combined with the other pile of random backlight issues the assumption that we do something (maybe something a bit racy) that windows doesn't do isn't too far-fetched. So I'm not wary of the machines where the aml is busted for acpi_os=win8, but for the others where this broke stuff.
Or do I miss something here?
The ACPI video driver doesn't do anything new now. It only does things that did work before we started to tell BIOSes we're compatible with Win8. The reason why they don't work on some machines now is not related to whether or not Win8 is broken, but to what is there in the ACPI tables on those machines. That *is* pure garbage, but Win8 users don't see that (presumably, because Win8 does't execute the AML in question). We don't see that either with acpi_osi="!Windows 2012" (because then we don't execute that AML either).
Whether or not Win8 is broken doesn't matter here. What matters is that we have to deal with broken AML somehow.
One way may be to tell everyone affected by this to pass acpi_osi="!Windows 2012" in the kernel command line or possibly create a blacklist of those machines in the kernel (which Felipe has been pushing for recently) and wash our hands clean of this, but that leaves some exceptionally bad taste in my mouth.
The alternative is to try to use native backlight in i915, which we did, but that didn't work on some machines. I don't think we will know why it didn't work there before we collect some more information and that's not possible without user testing. So, the idea is to make that testing possible without hacking the kernel and that's why we're introducing the new command line switch. And we're going to ask users to try it and report back.
Thanks, Rafael
On Mon, 09 Sep 2013, "Rafael J. Wysocki" rjw@sisk.pl wrote:
On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
Hi,
On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
Hi Aaaron,
Have we grown any clue meanwhile about which Intel boxes need this and for which we still need to keep the acpi backlight around?
First of all, there is a bunch of boxes where ACPI backlight works incorrectly because of the Win8 compatibility issue. [In short, if we say we are compatible with Win8, the backlight AML goes into a special code path that is broken on those machines. Presumably Win8 uses native backlight control on them and that AML code path is never executed there.] The collection of machines with this problem appears to be kind of random (various models from various vendors), but I *think* they are machines that originally shipped with Win7 with a Win8 "upgrade" option (which in practice requires the BIOS to be updated anyway).
Because of that, last time we tried to switch all of the systems using i915 and telling the BIOS that they are compatible with Win8 over to native backlight control, but that didn't work for two reasons. The first reason is that some user space doesn't know how to use intel_backlight and needs to be taught about that (so some systems are just not ready for that switch). The second and more fundamental reason is that the native backlight control simply doesn't work on some machines and we don't seem to have any idea why and how to debug this particular problem (mostly because we don't have enough information and we don't know what to ask for).
I've grown _very_ reluctant to just adding tons of quirks to our driver for the backlight.
Almost all the quirks we have added recently (or that have been proposed to be added) are for the backlight. Imo that indicates we get something fundamentally wrong ...
This thing isn't really a quirk. It rather is an option for the users of the systems where ACPI backlight doesn't work to switch over to the native backlight control using a command line switch. This way they can at least *see* if the native backlight control works for them and (hopefully) report problems if that's not the case. This gives us a chance to get more information about what the problem is and possibly to make some progress without making changes for everyone, reverting those changes when they don't work etc.
An alternative for them is to pass acpi_osi="!Windows 2012" which will probably make the ACPI backlight work for them again, but this rather is a step back, because we can't possibly learn anything new from that.
If Win8 is as broken as we are I'm ok with the module option. It just sounded to me like right now we don't know of a way to make all machines somewhat happy, combined with the other pile of random backlight issues the assumption that we do something (maybe something a bit racy) that windows doesn't do isn't too far-fetched. So I'm not wary of the machines where the aml is busted for acpi_os=win8, but for the others where this broke stuff.
Or do I miss something here?
The ACPI video driver doesn't do anything new now. It only does things that did work before we started to tell BIOSes we're compatible with Win8. The reason why they don't work on some machines now is not related to whether or not Win8 is broken, but to what is there in the ACPI tables on those machines. That *is* pure garbage, but Win8 users don't see that (presumably, because Win8 does't execute the AML in question). We don't see that either with acpi_osi="!Windows 2012" (because then we don't execute that AML either).
Whether or not Win8 is broken doesn't matter here. What matters is that we have to deal with broken AML somehow.
One way may be to tell everyone affected by this to pass acpi_osi="!Windows 2012" in the kernel command line or possibly create a blacklist of those machines in the kernel (which Felipe has been pushing for recently) and wash our hands clean of this, but that leaves some exceptionally bad taste in my mouth.
The alternative is to try to use native backlight in i915, which we did, but that didn't work on some machines. I don't think we will know why it didn't work there before we collect some more information and that's not possible without user testing. So, the idea is to make that testing possible without hacking the kernel and that's why we're introducing the new command line switch. And we're going to ask users to try it and report back.
The thing that slightly bugs me with the proposed patches is that they're adding a module parameter to i915 to tell ACPI video driver whether to quirk the backlight or not. Before you know, we *will* have requests to add quirks to i915 to tell ACPI video driver this.
I think the parameter "Does the ACPI backlight interface work or not" belongs to the ACPI video driver.
Feel free to file this in your bikeshedding folder, but I think i915 should only tell ACPI "I have a native backlight interface". What ACPI video driver does with that information is its business. The desired thing to do here would be to check if there's a module parameter or a quirk to disable the ACPI backlight interface. acpi_backlight=DTRT? ;)
BR, Jani.
On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote:
I think the parameter "Does the ACPI backlight interface work or not" belongs to the ACPI video driver.
That depends on how Windows 8 works. If Windows 8 policy is handled by the GPU drivers then it belongs in i915. If it's handled by the ACPI code then it belongs in the ACPI code. But I have no way of determining that, whereas you work for a company that produces a Windows 8 video driver…
On Tue, 10 Sep 2013, Matthew Garrett matthew.garrett@nebula.com wrote:
On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote:
I think the parameter "Does the ACPI backlight interface work or not" belongs to the ACPI video driver.
That depends on how Windows 8 works. If Windows 8 policy is handled by the GPU drivers then it belongs in i915. If it's handled by the ACPI code then it belongs in the ACPI code.
I fail to see the logic. Windows 8 policy dictates whether we can use the AML code or not. IMHO, ACPI code is in the best position to figure this out and quirk as necessary. It's the part that knows about Windows 8, not i915.
But I have no way of determining that, whereas you work for a company that produces a Windows 8 video driver…
Here I do see the logic. However see signature; I'm not in the best of positions to figure out things about Windows 8 video drivers. ;) But I'll see what I can do.
BR, Jani.
On Tue, 2013-09-10 at 17:21 +0300, Jani Nikula wrote:
On Tue, 10 Sep 2013, Matthew Garrett matthew.garrett@nebula.com wrote:
On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote:
I think the parameter "Does the ACPI backlight interface work or not" belongs to the ACPI video driver.
That depends on how Windows 8 works. If Windows 8 policy is handled by the GPU drivers then it belongs in i915. If it's handled by the ACPI code then it belongs in the ACPI code.
I fail to see the logic. Windows 8 policy dictates whether we can use the AML code or not. IMHO, ACPI code is in the best position to figure this out and quirk as necessary. It's the part that knows about Windows 8, not i915.
So if nvidia hardware uses the ACPI interface and Intel doesn't, we should still quirk it in the ACPI driver?
On Tuesday, September 10, 2013 04:53:40 PM Jani Nikula wrote:
On Mon, 09 Sep 2013, "Rafael J. Wysocki" rjw@sisk.pl wrote:
On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
Hi,
On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
Hi Aaaron,
Have we grown any clue meanwhile about which Intel boxes need this and for which we still need to keep the acpi backlight around?
First of all, there is a bunch of boxes where ACPI backlight works incorrectly because of the Win8 compatibility issue. [In short, if we say we are compatible with Win8, the backlight AML goes into a special code path that is broken on those machines. Presumably Win8 uses native backlight control on them and that AML code path is never executed there.] The collection of machines with this problem appears to be kind of random (various models from various vendors), but I *think* they are machines that originally shipped with Win7 with a Win8 "upgrade" option (which in practice requires the BIOS to be updated anyway).
Because of that, last time we tried to switch all of the systems using i915 and telling the BIOS that they are compatible with Win8 over to native backlight control, but that didn't work for two reasons. The first reason is that some user space doesn't know how to use intel_backlight and needs to be taught about that (so some systems are just not ready for that switch). The second and more fundamental reason is that the native backlight control simply doesn't work on some machines and we don't seem to have any idea why and how to debug this particular problem (mostly because we don't have enough information and we don't know what to ask for).
I've grown _very_ reluctant to just adding tons of quirks to our driver for the backlight.
Almost all the quirks we have added recently (or that have been proposed to be added) are for the backlight. Imo that indicates we get something fundamentally wrong ...
This thing isn't really a quirk. It rather is an option for the users of the systems where ACPI backlight doesn't work to switch over to the native backlight control using a command line switch. This way they can at least *see* if the native backlight control works for them and (hopefully) report problems if that's not the case. This gives us a chance to get more information about what the problem is and possibly to make some progress without making changes for everyone, reverting those changes when they don't work etc.
An alternative for them is to pass acpi_osi="!Windows 2012" which will probably make the ACPI backlight work for them again, but this rather is a step back, because we can't possibly learn anything new from that.
If Win8 is as broken as we are I'm ok with the module option. It just sounded to me like right now we don't know of a way to make all machines somewhat happy, combined with the other pile of random backlight issues the assumption that we do something (maybe something a bit racy) that windows doesn't do isn't too far-fetched. So I'm not wary of the machines where the aml is busted for acpi_os=win8, but for the others where this broke stuff.
Or do I miss something here?
The ACPI video driver doesn't do anything new now. It only does things that did work before we started to tell BIOSes we're compatible with Win8. The reason why they don't work on some machines now is not related to whether or not Win8 is broken, but to what is there in the ACPI tables on those machines. That *is* pure garbage, but Win8 users don't see that (presumably, because Win8 does't execute the AML in question). We don't see that either with acpi_osi="!Windows 2012" (because then we don't execute that AML either).
Whether or not Win8 is broken doesn't matter here. What matters is that we have to deal with broken AML somehow.
One way may be to tell everyone affected by this to pass acpi_osi="!Windows 2012" in the kernel command line or possibly create a blacklist of those machines in the kernel (which Felipe has been pushing for recently) and wash our hands clean of this, but that leaves some exceptionally bad taste in my mouth.
The alternative is to try to use native backlight in i915, which we did, but that didn't work on some machines. I don't think we will know why it didn't work there before we collect some more information and that's not possible without user testing. So, the idea is to make that testing possible without hacking the kernel and that's why we're introducing the new command line switch. And we're going to ask users to try it and report back.
The thing that slightly bugs me with the proposed patches is that they're adding a module parameter to i915 to tell ACPI video driver whether to quirk the backlight or not. Before you know, we *will* have requests to add quirks to i915 to tell ACPI video driver this.
I think the parameter "Does the ACPI backlight interface work or not" belongs to the ACPI video driver.
Feel free to file this in your bikeshedding folder, but I think i915 should only tell ACPI "I have a native backlight interface".
It kind of does that already through the intel_opregion_init() thing, so it would be trivial to test intel_opregion_present() in acpi_video_register().
So yes, we can add a command line option, say 'use_native_backlight' to the ACPI video driver that will work like this: "if the Intel opregion is present and video.use_native_backlight is set, skip registering ACPI backlight".
Is that what you want?
Rafael
On Tue, Sep 10, 2013 at 09:23:04PM +0200, Rafael J. Wysocki wrote:
On Tuesday, September 10, 2013 04:53:40 PM Jani Nikula wrote:
On Mon, 09 Sep 2013, "Rafael J. Wysocki" rjw@sisk.pl wrote:
On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
Hi,
On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
Hi Aaaron,
Have we grown any clue meanwhile about which Intel boxes need this and for which we still need to keep the acpi backlight around?
First of all, there is a bunch of boxes where ACPI backlight works incorrectly because of the Win8 compatibility issue. [In short, if we say we are compatible with Win8, the backlight AML goes into a special code path that is broken on those machines. Presumably Win8 uses native backlight control on them and that AML code path is never executed there.] The collection of machines with this problem appears to be kind of random (various models from various vendors), but I *think* they are machines that originally shipped with Win7 with a Win8 "upgrade" option (which in practice requires the BIOS to be updated anyway).
Because of that, last time we tried to switch all of the systems using i915 and telling the BIOS that they are compatible with Win8 over to native backlight control, but that didn't work for two reasons. The first reason is that some user space doesn't know how to use intel_backlight and needs to be taught about that (so some systems are just not ready for that switch). The second and more fundamental reason is that the native backlight control simply doesn't work on some machines and we don't seem to have any idea why and how to debug this particular problem (mostly because we don't have enough information and we don't know what to ask for).
I've grown _very_ reluctant to just adding tons of quirks to our driver for the backlight.
Almost all the quirks we have added recently (or that have been proposed to be added) are for the backlight. Imo that indicates we get something fundamentally wrong ...
This thing isn't really a quirk. It rather is an option for the users of the systems where ACPI backlight doesn't work to switch over to the native backlight control using a command line switch. This way they can at least *see* if the native backlight control works for them and (hopefully) report problems if that's not the case. This gives us a chance to get more information about what the problem is and possibly to make some progress without making changes for everyone, reverting those changes when they don't work etc.
An alternative for them is to pass acpi_osi="!Windows 2012" which will probably make the ACPI backlight work for them again, but this rather is a step back, because we can't possibly learn anything new from that.
If Win8 is as broken as we are I'm ok with the module option. It just sounded to me like right now we don't know of a way to make all machines somewhat happy, combined with the other pile of random backlight issues the assumption that we do something (maybe something a bit racy) that windows doesn't do isn't too far-fetched. So I'm not wary of the machines where the aml is busted for acpi_os=win8, but for the others where this broke stuff.
Or do I miss something here?
The ACPI video driver doesn't do anything new now. It only does things that did work before we started to tell BIOSes we're compatible with Win8. The reason why they don't work on some machines now is not related to whether or not Win8 is broken, but to what is there in the ACPI tables on those machines. That *is* pure garbage, but Win8 users don't see that (presumably, because Win8 does't execute the AML in question). We don't see that either with acpi_osi="!Windows 2012" (because then we don't execute that AML either).
Whether or not Win8 is broken doesn't matter here. What matters is that we have to deal with broken AML somehow.
One way may be to tell everyone affected by this to pass acpi_osi="!Windows 2012" in the kernel command line or possibly create a blacklist of those machines in the kernel (which Felipe has been pushing for recently) and wash our hands clean of this, but that leaves some exceptionally bad taste in my mouth.
The alternative is to try to use native backlight in i915, which we did, but that didn't work on some machines. I don't think we will know why it didn't work there before we collect some more information and that's not possible without user testing. So, the idea is to make that testing possible without hacking the kernel and that's why we're introducing the new command line switch. And we're going to ask users to try it and report back.
The thing that slightly bugs me with the proposed patches is that they're adding a module parameter to i915 to tell ACPI video driver whether to quirk the backlight or not. Before you know, we *will* have requests to add quirks to i915 to tell ACPI video driver this.
I think the parameter "Does the ACPI backlight interface work or not" belongs to the ACPI video driver.
Feel free to file this in your bikeshedding folder, but I think i915 should only tell ACPI "I have a native backlight interface".
It kind of does that already through the intel_opregion_init() thing, so it would be trivial to test intel_opregion_present() in acpi_video_register().
It is possible the i915 driver decides not to register a backlight interface for the graphics card for some reason(memory allocation failed or it knows the native control does not work on this card or whatever), so I would prefer let i915 tell ACPI video that it has registered a native backlight control interface as Jani has said.
Then together with the video.use_native_backlight, we can register or not register ACPI video backlight interface accordingly. Or rather, we can simply not register ACPI video backlight interface for Win8 systems as long as i915 indicates that it has native backlight control(if the native control is broken, i915 should fix it or blacklist it so that i915 will not indicate it has native backlight control and ACPI video will continue to register its own).
How does this sound?
-Aaron
So yes, we can add a command line option, say 'use_native_backlight' to the ACPI video driver that will work like this: "if the Intel opregion is present and video.use_native_backlight is set, skip registering ACPI backlight".
Is that what you want?
Rafael
-- 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 Wed, 11 Sep 2013, Aaron Lu aaron.lu@intel.com wrote:
It is possible the i915 driver decides not to register a backlight interface for the graphics card for some reason(memory allocation failed or it knows the native control does not work on this card or whatever), so I would prefer let i915 tell ACPI video that it has registered a native backlight control interface as Jani has said.
Then together with the video.use_native_backlight, we can register or not register ACPI video backlight interface accordingly. Or rather, we can simply not register ACPI video backlight interface for Win8 systems as long as i915 indicates that it has native backlight control(if the native control is broken, i915 should fix it or blacklist it so that i915 will not indicate it has native backlight control and ACPI video will continue to register its own).
How does this sound?
Sounds good to me.
Before plunging forward, have you observed any difference between the boot modes? We have reports [1] that the backlight behaviour is different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole story.
Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code paths, what guarantees do we have of UEFI+CSM or legacy boots working?
BR, Jani.
[1] https://bugzilla.kernel.org/show_bug.cgi?id=47941#c96
On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:
Before plunging forward, have you observed any difference between the boot modes? We have reports [1] that the backlight behaviour is different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole story.
Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code paths, what guarantees do we have of UEFI+CSM or legacy boots working?
We have no evidence of Windows behaving differently based on the exposed firmware type.
On mer., 2013-09-11 at 08:45 +0000, Matthew Garrett wrote:
On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:
Before plunging forward, have you observed any difference between the boot modes? We have reports [1] that the backlight behaviour is different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole story.
Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code paths, what guarantees do we have of UEFI+CSM or legacy boots working?
We have no evidence of Windows behaving differently based on the exposed firmware type.
I do have an X230 which boots fine using pure UEFI or UEFI+CSM (and I guess I can try booting it with a grml for pure legacy), so I can do tests if needed.
Regards,
On Wed, 11 Sep 2013, Matthew Garrett matthew.garrett@nebula.com wrote:
On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:
Before plunging forward, have you observed any difference between the boot modes? We have reports [1] that the backlight behaviour is different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole story.
Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code paths, what guarantees do we have of UEFI+CSM or legacy boots working?
We have no evidence of Windows behaving differently based on the exposed firmware type.
By "behaving differently", do you mean internally adapting to the boot mode, or exhibiting different behaviour to the user?
We have evidence of the firmware behaving differently (VBT, backlight) based on the boot mode, all else being equal. We don't adapt to that, and we fail. I don't know if we should adapt, or do things differently altogether. I don't even know if Windows 8 works on all boot modes on the machines in question.
BR, Jani.
On Wed, 2013-09-11 at 13:29 +0300, Jani Nikula wrote:
On Wed, 11 Sep 2013, Matthew Garrett matthew.garrett@nebula.com wrote:
On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:
Before plunging forward, have you observed any difference between the boot modes? We have reports [1] that the backlight behaviour is different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole story.
Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code paths, what guarantees do we have of UEFI+CSM or legacy boots working?
We have no evidence of Windows behaving differently based on the exposed firmware type.
By "behaving differently", do you mean internally adapting to the boot mode, or exhibiting different behaviour to the user?
As far as backlight control goes, both.
We have evidence of the firmware behaving differently (VBT, backlight) based on the boot mode, all else being equal. We don't adapt to that, and we fail. I don't know if we should adapt, or do things differently altogether. I don't even know if Windows 8 works on all boot modes on the machines in question.
Sure, but Windows knows nothing about VBT or opregion-backed backlight control. That's up to the Intel driver.
On Wed, Sep 11, 2013 at 11:45:19AM +0300, Jani Nikula wrote:
On Wed, 11 Sep 2013, Aaron Lu aaron.lu@intel.com wrote:
It is possible the i915 driver decides not to register a backlight interface for the graphics card for some reason(memory allocation failed or it knows the native control does not work on this card or whatever), so I would prefer let i915 tell ACPI video that it has registered a native backlight control interface as Jani has said.
Then together with the video.use_native_backlight, we can register or not register ACPI video backlight interface accordingly. Or rather, we can simply not register ACPI video backlight interface for Win8 systems as long as i915 indicates that it has native backlight control(if the native control is broken, i915 should fix it or blacklist it so that i915 will not indicate it has native backlight control and ACPI video will continue to register its own).
How does this sound?
Sounds good to me.
Before plunging forward, have you observed any difference between the boot modes? We have reports [1] that the backlight behaviour is
Not yet from ACPI's point of view.
different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole story.
This check in patch 2/2 is a policy: for Win8 system, we think the native backlight control has a better chance of working than the ACPI video's, so I think the check is enough in ACPI video.
Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code paths, what guarantees do we have of UEFI+CSM or legacy boots working?
I suppose the 'tested BIOS code paths' means the pure UEFI boot mode? I don't know what guatantees do we have since I don't know what happened underneath after the backlight register is set in i915 driver, you or other i915 driver people should know more than I do :-)
BTW, after the backlight register is set in i915, is it that some find of firmware code will run in response to the setting of the register(e.g. the BLC_PWM_CTL/BLC_PWM_CPU_CTL/PCI_LBPC reg)?
Thanks, Aaron
On Mon, Sep 09, 2013 at 11:32:10AM +0200, Daniel Vetter wrote:
Hi Aaaron,
Have we grown any clue meanwhile about which Intel boxes need this and for which we still need to keep the acpi backlight around? I've grown _very_
Sorry, no general rule has been found. As Rafael has said, it appears to be random... And in addition to the "shipped with Win7 with a Win8 upgrade option" case, I also find a Sony laptop that has Win8 pre-installed and a broken ACPI video backlight interface. Its ACPI video backlight control method is simply a stub, so even the acpi_osi="!Windows 2012" will not provide a working backlight for this system.
-Aaron
reluctant to just adding tons of quirks to our driver for the backlight. Almost all the quirks we have added recently (or that have been proposed to be added) are for the backlight. Imo that indicates we get something fundamentally wrong ...
Cheers, Daniel
On Mon, Sep 09, 2013 at 04:42:20PM +0800, Aaron Lu wrote:
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(bool), that if ture is passed, 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() instead of acpi_video_register() in i915_driver_load(), and the param passed there is controlled by the i915 module level parameter i915_take_over_backlight, which is set to false by default.
This change is evolved from earlier patches of Matthew Garrett, Chun-Yi Lee and Seth Forshee and is heavily based on two patches from Rafael: https://lkml.org/lkml/2013/7/17/720 https://lkml.org/lkml/2013/7/24/806
Signed-off-by: Aaron Lu aaron.lu@intel.com
drivers/acpi/internal.h | 2 ++ drivers/acpi/video.c | 24 ++++++++++++++++-------- drivers/acpi/video_detect.c | 15 ++++++++++++++- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 5 +++++ drivers/gpu/drm/i915/i915_drv.h | 1 + include/acpi/video.h | 9 +++++++-- include/linux/acpi.h | 1 + 8 files changed, 47 insertions(+), 12 deletions(-)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 20f4233..a53832e 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev, -------------------------------------------------------------------------- */ #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_ */ diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 5ad5a71..cc709a7 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event) 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)
@@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
static void acpi_video_dev_register_backlight(struct acpi_video_device *device) {
- if (acpi_video_backlight_support()) {
- if (acpi_video_verify_backlight_support()) { struct backlight_properties props; struct pci_dev *pdev; acpi_handle acpi_parent;
@@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void) 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_video_unregister_backlight();- return 0; }
@@ -1959,7 +1967,7 @@ int acpi_video_register(void)
return 0; } -EXPORT_SYMBOL(acpi_video_register); +EXPORT_SYMBOL(__acpi_video_register);
void acpi_video_unregister(void) { diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 940edbf..c7e43ec 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void)
bool acpi_video_backlight_quirks(void) {
- return acpi_gbl_osi_data >= ACPI_OSI_WIN_8;
- 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);
@@ -283,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.
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f466980..75fba17 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (INTEL_INFO(dev)->num_pipes) { /* Must be done after probing outputs */ intel_opregion_init(dev);
acpi_video_register();
__acpi_video_register(i915_take_over_backlight);}
if (IS_GEN5(dev))
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 45b3c03..b014398 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -132,6 +132,11 @@ int i915_enable_ips __read_mostly = 1; module_param_named(enable_ips, i915_enable_ips, int, 0600); MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
+bool i915_take_over_backlight = false; +module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644); +MODULE_PARM_DESC(take_over_backlight,
"Prevent ACPI backlight from being used (default: false)");static struct drm_driver driver; extern int intel_agp_enabled;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1929bff..b43c430 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly; extern unsigned int i915_preliminary_hw_support __read_mostly; extern int i915_disable_power_well __read_mostly; extern int i915_enable_ips __read_mostly; +extern bool i915_take_over_backlight __read_mostly;
extern int i915_suspend(struct drm_device *dev, pm_message_t state); extern int i915_resume(struct drm_device *dev); diff --git a/include/acpi/video.h b/include/acpi/video.h index 0c665b5..dc5b8ad 100644 --- a/include/acpi/video.h +++ b/include/acpi/video.h @@ -17,13 +17,13 @@ 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); extern void acpi_video_unregister(void); extern int acpi_video_unregister_backlight(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(bool backlight_quirks) { return 0; } static inline void acpi_video_unregister(void) { return; } static inline int acpi_video_unregister_backlight(void) { return; } static inline int acpi_video_get_edid(struct acpi_device *device, int type, @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type, } #endif
+static inline int acpi_video_register(void) +{
- return __acpi_video_register(false);
+}
#endif diff --git a/include/linux/acpi.h b/include/linux/acpi.h index a5db4ae..a0dcce5 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid); #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)
-- 1.8.4.12.g2ea3df6
-- Daniel Vetter Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
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 Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f466980..75fba17 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (INTEL_INFO(dev)->num_pipes) { /* Must be done after probing outputs */ intel_opregion_init(dev);
acpi_video_register();
__acpi_video_register(i915_take_over_backlight);}
if (IS_GEN5(dev))
I can't compile:
DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load': DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit declaration of function '__acpi_video_register' [-Werror=implicit-function-declaration] DEBUG: __acpi_video_register(i915_take_over_backlight); DEBUG: ^ DEBUG: cc1: some warnings being treated as errors DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1 DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2 DEBUG: make[2]: *** [drivers/gpu/drm] Error 2 DEBUG: make[1]: *** [drivers/gpu] Error 2 DEBUG: make: *** [drivers] Error 2
On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f466980..75fba17 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (INTEL_INFO(dev)->num_pipes) { /* Must be done after probing outputs */ intel_opregion_init(dev);
acpi_video_register();
__acpi_video_register(i915_take_over_backlight);}
if (IS_GEN5(dev))
I can't compile:
DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load': DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit declaration of function '__acpi_video_register' [-Werror=implicit-function-declaration] DEBUG: __acpi_video_register(i915_take_over_backlight); DEBUG: ^ DEBUG: cc1: some warnings being treated as errors DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1 DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2 DEBUG: make[2]: *** [drivers/gpu/drm] Error 2 DEBUG: make[1]: *** [drivers/gpu] Error 2 DEBUG: make: *** [drivers] Error 2
The two patches are based on top of Rafael's linux-next tree. I just tried it again, no compile problem for me. I also tried on today Linus' master tree, as there are some updates from i915, two conflicts exist. I've just resolved them and will update it in next revision. If you want to try it now, please use: https://github.com/aaronlu/linux acpi_video_rework
Thanks, Aaron
On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote:
On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f466980..75fba17 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (INTEL_INFO(dev)->num_pipes) { /* Must be done after probing outputs */ intel_opregion_init(dev);
acpi_video_register();
__acpi_video_register(i915_take_over_backlight);}
if (IS_GEN5(dev))
I can't compile:
DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load': DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit declaration of function '__acpi_video_register' [-Werror=implicit-function-declaration] DEBUG: __acpi_video_register(i915_take_over_backlight); DEBUG: ^ DEBUG: cc1: some warnings being treated as errors DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1 DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2 DEBUG: make[2]: *** [drivers/gpu/drm] Error 2 DEBUG: make[1]: *** [drivers/gpu] Error 2 DEBUG: make: *** [drivers] Error 2
The two patches are based on top of Rafael's linux-next tree. I just tried it again, no compile problem for me. I also tried on today Linus' master tree, as there are some updates from i915, two conflicts exist. I've just resolved them and will update it in next revision. If you want to try it now, please use: https://github.com/aaronlu/linux acpi_video_rework
Thanks, Aaron
Thanks. this patch fixes my problems w/ compilation. I've tested this two patches and after apply I have: $ tree /sys/class/backlight/ /sys/class/backlight/ |-- acpi_video0 -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0 `-- intel_backlight -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
2 directories, 0 files
I think it's didn't unregistered.. I may forget. I need to apply one of patch from Matthew ?
Some strings from logs: DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013 thinkpad_acpi: Standard ACPI backlight interface available, not loading native one
On 09/10/2013 01:13 PM, Igor Gnatenko wrote:
On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote:
On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f466980..75fba17 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (INTEL_INFO(dev)->num_pipes) { /* Must be done after probing outputs */ intel_opregion_init(dev);
acpi_video_register();
__acpi_video_register(i915_take_over_backlight);}
if (IS_GEN5(dev))
I can't compile:
DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load': DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit declaration of function '__acpi_video_register' [-Werror=implicit-function-declaration] DEBUG: __acpi_video_register(i915_take_over_backlight); DEBUG: ^ DEBUG: cc1: some warnings being treated as errors DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1 DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2 DEBUG: make[2]: *** [drivers/gpu/drm] Error 2 DEBUG: make[1]: *** [drivers/gpu] Error 2 DEBUG: make: *** [drivers] Error 2
The two patches are based on top of Rafael's linux-next tree. I just tried it again, no compile problem for me. I also tried on today Linus' master tree, as there are some updates from i915, two conflicts exist. I've just resolved them and will update it in next revision. If you want to try it now, please use: https://github.com/aaronlu/linux acpi_video_rework
Thanks, Aaron
Thanks. this patch fixes my problems w/ compilation. I've tested this two patches and after apply I have: $ tree /sys/class/backlight/ /sys/class/backlight/ |-- acpi_video0 -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0 `-- intel_backlight -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
2 directories, 0 files
I think it's didn't unregistered.. I may forget. I need to apply one of patch from Matthew ?
You need to specify i915.take_over_backlight=1 in kernel cmdline, that module option is set to false by default for now.
Thanks for the test.
-Aaron
Some strings from logs: DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013 thinkpad_acpi: Standard ACPI backlight interface available, not loading native one
On Tue, 2013-09-10 at 13:16 +0800, Aaron Lu wrote:
On 09/10/2013 01:13 PM, Igor Gnatenko wrote:
On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote:
On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f466980..75fba17 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (INTEL_INFO(dev)->num_pipes) { /* Must be done after probing outputs */ intel_opregion_init(dev);
acpi_video_register();
__acpi_video_register(i915_take_over_backlight);}
if (IS_GEN5(dev))
I can't compile:
DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load': DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit declaration of function '__acpi_video_register' [-Werror=implicit-function-declaration] DEBUG: __acpi_video_register(i915_take_over_backlight); DEBUG: ^ DEBUG: cc1: some warnings being treated as errors DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1 DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2 DEBUG: make[2]: *** [drivers/gpu/drm] Error 2 DEBUG: make[1]: *** [drivers/gpu] Error 2 DEBUG: make: *** [drivers] Error 2
The two patches are based on top of Rafael's linux-next tree. I just tried it again, no compile problem for me. I also tried on today Linus' master tree, as there are some updates from i915, two conflicts exist. I've just resolved them and will update it in next revision. If you want to try it now, please use: https://github.com/aaronlu/linux acpi_video_rework
Thanks, Aaron
Thanks. this patch fixes my problems w/ compilation. I've tested this two patches and after apply I have: $ tree /sys/class/backlight/ /sys/class/backlight/ |-- acpi_video0 -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0 `-- intel_backlight -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
2 directories, 0 files
I think it's didn't unregistered.. I may forget. I need to apply one of patch from Matthew ?
You need to specify i915.take_over_backlight=1 in kernel cmdline, that module option is set to false by default for now.
Thanks for the test.
-Aaron
Some strings from logs: DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013 thinkpad_acpi: Standard ACPI backlight interface available, not loading native one
Thanks for quick answer. Yes. This option do unregister. Thanks. but for this patch-set I also need "[PATCH 2/3] ACPI / video: Always call acpi_video_init_brightness() on init" from Matthew (for notifications in DE).
On 09/10/2013 01:22 PM, Igor Gnatenko wrote:
On Tue, 2013-09-10 at 13:16 +0800, Aaron Lu wrote:
On 09/10/2013 01:13 PM, Igor Gnatenko wrote:
On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote:
On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f466980..75fba17 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) if (INTEL_INFO(dev)->num_pipes) { /* Must be done after probing outputs */ intel_opregion_init(dev);
acpi_video_register();
__acpi_video_register(i915_take_over_backlight);}
if (IS_GEN5(dev))
I can't compile:
DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load': DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit declaration of function '__acpi_video_register' [-Werror=implicit-function-declaration] DEBUG: __acpi_video_register(i915_take_over_backlight); DEBUG: ^ DEBUG: cc1: some warnings being treated as errors DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1 DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2 DEBUG: make[2]: *** [drivers/gpu/drm] Error 2 DEBUG: make[1]: *** [drivers/gpu] Error 2 DEBUG: make: *** [drivers] Error 2
The two patches are based on top of Rafael's linux-next tree. I just tried it again, no compile problem for me. I also tried on today Linus' master tree, as there are some updates from i915, two conflicts exist. I've just resolved them and will update it in next revision. If you want to try it now, please use: https://github.com/aaronlu/linux acpi_video_rework
Thanks, Aaron
Thanks. this patch fixes my problems w/ compilation. I've tested this two patches and after apply I have: $ tree /sys/class/backlight/ /sys/class/backlight/ |-- acpi_video0 -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0 `-- intel_backlight -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
2 directories, 0 files
I think it's didn't unregistered.. I may forget. I need to apply one of patch from Matthew ?
You need to specify i915.take_over_backlight=1 in kernel cmdline, that module option is set to false by default for now.
Thanks for the test.
-Aaron
Some strings from logs: DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013 thinkpad_acpi: Standard ACPI backlight interface available, not loading native one
Thanks for quick answer. Yes. This option do unregister. Thanks. but for this patch-set I also need "[PATCH 2/3] ACPI / video: Always call acpi_video_init_brightness() on init" from Matthew (for notifications in DE).
That patch is reverted as it cause problem for other system: https://bugs.freedesktop.org/show_bug.cgi?id=68355
OTOH, the thinkpad-acpi module already has a call to _BCL except that the tpacpi_acpi_handle_locate failed to locate video controller's handle: https://bugzilla.kernel.org/show_bug.cgi?id=51231#c121 I'll see if I can figure out why.
Thanks, Aaron
On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
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(bool), that if ture is passed, 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() instead of acpi_video_register() in i915_driver_load(), and the param passed there is controlled by the i915 module level parameter i915_take_over_backlight, which is set to false by default.
This change is evolved from earlier patches of Matthew Garrett, Chun-Yi Lee and Seth Forshee and is heavily based on two patches from Rafael: https://lkml.org/lkml/2013/7/17/720 https://lkml.org/lkml/2013/7/24/806
Signed-off-by: Aaron Lu aaron.lu@intel.com
Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com
drivers/acpi/internal.h | 2 ++ drivers/acpi/video.c | 24 ++++++++++++++++-------- drivers/acpi/video_detect.c | 15 ++++++++++++++- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 5 +++++ drivers/gpu/drm/i915/i915_drv.h | 1 + include/acpi/video.h | 9 +++++++-- include/linux/acpi.h | 1 + 8 files changed, 47 insertions(+), 12 deletions(-)
dri-devel@lists.freedesktop.org
-
Aaron Lu -
Daniel Vetter -
Igor Gnatenko -
Jani Nikula -
Matthew Garrett -
Rafael J. Wysocki -
Yves-Alexis Perez