v4: Remove decleration and stub for acpi_video_unregister_backlight in video.h of patch 2/4 since that function doesn't exist anymore in v3.
v3: 1 Add a new patch 4/4 to fix some problems in thinkpad-acpi module; 2 Remove unnecessary function acpi_video_unregister_backlight introduced in patch 2/4 as pointed out by Jani Nikula.
v2: v1 has the subject of "Rework ACPI video driver" and is posted here: http://lkml.org/lkml/2013/9/9/74 Since the objective is really to fix Win8 backlight issues, I changed the subject in this version, sorry about that.
This patchset has four patches, the first introduced a new API named backlight_device_registered in backlight layer that can be used for backlight interface provider module to check if a specific type backlight interface has been registered, see changelog for patch 1/4 for details. Then patch 2/4 does the cleanup to sepeate the backlight control and event delivery functionality in the ACPI video module and patch 3/4 solves some Win8 backlight control problems by avoiding register ACPI video's backlight interface if: 1 Kernel cmdline option acpi_backlight=video is not given; 2 This is a Win8 system; 3 Native backlight control interface exists. Patch 4/4 fixes some problems in thinkpad-acpi module.
Technically, patch 2/4 is not required to fix the issue here. So if you think it is not necessary, I can remove it from the series.
Aaron Lu (4): backlight: introduce backlight_device_registered ACPI / video: seperate backlight control and event interface ACPI / video: Do not register backlight if win8 and native interface exists thinkpad-acpi: fix handle locate for video and query of _BCL
drivers/acpi/internal.h | 5 +- drivers/acpi/video.c | 442 ++++++++++++++++++++--------------- drivers/acpi/video_detect.c | 14 +- drivers/platform/x86/thinkpad_acpi.c | 31 ++- drivers/video/backlight/backlight.c | 31 +++ include/linux/backlight.h | 4 + 6 files changed, 322 insertions(+), 205 deletions(-)
Introduce a new API for modules to query if a specific type of backlight device has been registered. This is useful for some backlight device provider module(e.g. ACPI video) to know if a native control interface(e.g. the interface created by i915) is available and then do things accordingly(e.g. avoid register its own on Win8 systems).
Signed-off-by: Aaron Lu aaron.lu@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com Tested-by: Yves-Alexis Perez corsac@debian.org Tested-by: Mika Westerberg mika.westerberg@linux.intel.com --- drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++++ include/linux/backlight.h | 4 ++++ 2 files changed, 35 insertions(+)
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c index 94a403a..bf2d71d 100644 --- a/drivers/video/backlight/backlight.c +++ b/drivers/video/backlight/backlight.c @@ -21,6 +21,9 @@ #include <asm/backlight.h> #endif
+static struct list_head bd_list_head; +static struct mutex bd_list_mutex; + static const char *const backlight_types[] = { [BACKLIGHT_RAW] = "raw", [BACKLIGHT_PLATFORM] = "platform", @@ -349,10 +352,32 @@ struct backlight_device *backlight_device_register(const char *name, mutex_unlock(&pmac_backlight_mutex); #endif
+ mutex_lock(&bd_list_mutex); + list_add(&new_bd->entry, &bd_list_head); + mutex_unlock(&bd_list_mutex); + return new_bd; } EXPORT_SYMBOL(backlight_device_register);
+bool backlight_device_registered(enum backlight_type type) +{ + bool found = false; + struct backlight_device *bd; + + mutex_lock(&bd_list_mutex); + list_for_each_entry(bd, &bd_list_head, entry) { + if (bd->props.type == type) { + found = true; + break; + } + } + mutex_unlock(&bd_list_mutex); + + return found; +} +EXPORT_SYMBOL(backlight_device_registered); + /** * backlight_device_unregister - unregisters a backlight device object. * @bd: the backlight device object to be unregistered and freed. @@ -364,6 +389,10 @@ void backlight_device_unregister(struct backlight_device *bd) if (!bd) return;
+ mutex_lock(&bd_list_mutex); + list_del(&bd->entry); + mutex_unlock(&bd_list_mutex); + #ifdef CONFIG_PMAC_BACKLIGHT mutex_lock(&pmac_backlight_mutex); if (pmac_backlight == bd) @@ -499,6 +528,8 @@ static int __init backlight_class_init(void)
backlight_class->dev_groups = bl_device_groups; backlight_class->pm = &backlight_class_dev_pm_ops; + INIT_LIST_HEAD(&bd_list_head); + mutex_init(&bd_list_mutex); return 0; }
diff --git a/include/linux/backlight.h b/include/linux/backlight.h index 53b7794..5f9cd96 100644 --- a/include/linux/backlight.h +++ b/include/linux/backlight.h @@ -100,6 +100,9 @@ struct backlight_device { /* The framebuffer notifier block */ struct notifier_block fb_notif;
+ /* list entry of all registered backlight devices */ + struct list_head entry; + struct device dev; };
@@ -123,6 +126,7 @@ extern void devm_backlight_device_unregister(struct device *dev, struct backlight_device *bd); extern void backlight_force_update(struct backlight_device *bd, enum backlight_update_reason reason); +extern bool backlight_device_registered(enum backlight_type type);
#define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
On Tuesday, October 08, 2013 02:39:58 PM Aaron Lu wrote:
Introduce a new API for modules to query if a specific type of backlight device has been registered. This is useful for some backlight device provider module(e.g. ACPI video) to know if a native control interface(e.g. the interface created by i915) is available and then do things accordingly(e.g. avoid register its own on Win8 systems).
Signed-off-by: Aaron Lu aaron.lu@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com Tested-by: Yves-Alexis Perez corsac@debian.org Tested-by: Mika Westerberg mika.westerberg@linux.intel.com
drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++++ include/linux/backlight.h | 4 ++++ 2 files changed, 35 insertions(+)
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c index 94a403a..bf2d71d 100644 --- a/drivers/video/backlight/backlight.c +++ b/drivers/video/backlight/backlight.c @@ -21,6 +21,9 @@ #include <asm/backlight.h> #endif
+static struct list_head bd_list_head; +static struct mutex bd_list_mutex;
I'd prefer these two things to be called backlight_dev_list and backlight_dev_list_mutex, respectively.
static const char *const backlight_types[] = { [BACKLIGHT_RAW] = "raw", [BACKLIGHT_PLATFORM] = "platform", @@ -349,10 +352,32 @@ struct backlight_device *backlight_device_register(const char *name, mutex_unlock(&pmac_backlight_mutex); #endif
- mutex_lock(&bd_list_mutex);
- list_add(&new_bd->entry, &bd_list_head);
- mutex_unlock(&bd_list_mutex);
- return new_bd;
} EXPORT_SYMBOL(backlight_device_register);
+bool backlight_device_registered(enum backlight_type type) +{
- bool found = false;
- struct backlight_device *bd;
- mutex_lock(&bd_list_mutex);
- list_for_each_entry(bd, &bd_list_head, entry) {
if (bd->props.type == type) {found = true;break;}- }
Isn't it useful to be able to register more than one backlight device of the same type sometimes?
- mutex_unlock(&bd_list_mutex);
- return found;
+} +EXPORT_SYMBOL(backlight_device_registered);
/**
- backlight_device_unregister - unregisters a backlight device object.
- @bd: the backlight device object to be unregistered and freed.
@@ -364,6 +389,10 @@ void backlight_device_unregister(struct backlight_device *bd) if (!bd) return;
- mutex_lock(&bd_list_mutex);
- list_del(&bd->entry);
- mutex_unlock(&bd_list_mutex);
#ifdef CONFIG_PMAC_BACKLIGHT mutex_lock(&pmac_backlight_mutex); if (pmac_backlight == bd) @@ -499,6 +528,8 @@ static int __init backlight_class_init(void)
backlight_class->dev_groups = bl_device_groups; backlight_class->pm = &backlight_class_dev_pm_ops;
- INIT_LIST_HEAD(&bd_list_head);
- mutex_init(&bd_list_mutex); return 0;
}
diff --git a/include/linux/backlight.h b/include/linux/backlight.h index 53b7794..5f9cd96 100644 --- a/include/linux/backlight.h +++ b/include/linux/backlight.h @@ -100,6 +100,9 @@ struct backlight_device { /* The framebuffer notifier block */ struct notifier_block fb_notif;
- /* list entry of all registered backlight devices */
- struct list_head entry;
- struct device dev;
};
@@ -123,6 +126,7 @@ extern void devm_backlight_device_unregister(struct device *dev, struct backlight_device *bd); extern void backlight_force_update(struct backlight_device *bd, enum backlight_update_reason reason); +extern bool backlight_device_registered(enum backlight_type type);
#define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
Thanks!
On 10/10/2013 08:25 AM, Rafael J. Wysocki wrote:
On Tuesday, October 08, 2013 02:39:58 PM Aaron Lu wrote:
Introduce a new API for modules to query if a specific type of backlight device has been registered. This is useful for some backlight device provider module(e.g. ACPI video) to know if a native control interface(e.g. the interface created by i915) is available and then do things accordingly(e.g. avoid register its own on Win8 systems).
Signed-off-by: Aaron Lu aaron.lu@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com Tested-by: Yves-Alexis Perez corsac@debian.org Tested-by: Mika Westerberg mika.westerberg@linux.intel.com
drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++++ include/linux/backlight.h | 4 ++++ 2 files changed, 35 insertions(+)
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c index 94a403a..bf2d71d 100644 --- a/drivers/video/backlight/backlight.c +++ b/drivers/video/backlight/backlight.c @@ -21,6 +21,9 @@ #include <asm/backlight.h> #endif
+static struct list_head bd_list_head; +static struct mutex bd_list_mutex;
I'd prefer these two things to be called backlight_dev_list and backlight_dev_list_mutex, respectively.
OK.
static const char *const backlight_types[] = { [BACKLIGHT_RAW] = "raw", [BACKLIGHT_PLATFORM] = "platform", @@ -349,10 +352,32 @@ struct backlight_device *backlight_device_register(const char *name, mutex_unlock(&pmac_backlight_mutex); #endif
- mutex_lock(&bd_list_mutex);
- list_add(&new_bd->entry, &bd_list_head);
- mutex_unlock(&bd_list_mutex);
- return new_bd;
} EXPORT_SYMBOL(backlight_device_register);
+bool backlight_device_registered(enum backlight_type type) +{
- bool found = false;
- struct backlight_device *bd;
- mutex_lock(&bd_list_mutex);
- list_for_each_entry(bd, &bd_list_head, entry) {
if (bd->props.type == type) {found = true;break;}- }
Isn't it useful to be able to register more than one backlight device of the same type sometimes?
I think so for some kind of computers. OTOH, the above function should be enough for the problem we are solving here, if someday we need to differentiate, we can enhance the code then.
- mutex_unlock(&bd_list_mutex);
- return found;
+} +EXPORT_SYMBOL(backlight_device_registered);
/**
- backlight_device_unregister - unregisters a backlight device object.
- @bd: the backlight device object to be unregistered and freed.
@@ -364,6 +389,10 @@ void backlight_device_unregister(struct backlight_device *bd) if (!bd) return;
- mutex_lock(&bd_list_mutex);
- list_del(&bd->entry);
- mutex_unlock(&bd_list_mutex);
#ifdef CONFIG_PMAC_BACKLIGHT mutex_lock(&pmac_backlight_mutex); if (pmac_backlight == bd) @@ -499,6 +528,8 @@ static int __init backlight_class_init(void)
backlight_class->dev_groups = bl_device_groups; backlight_class->pm = &backlight_class_dev_pm_ops;
- INIT_LIST_HEAD(&bd_list_head);
- mutex_init(&bd_list_mutex); return 0;
}
diff --git a/include/linux/backlight.h b/include/linux/backlight.h index 53b7794..5f9cd96 100644 --- a/include/linux/backlight.h +++ b/include/linux/backlight.h @@ -100,6 +100,9 @@ struct backlight_device { /* The framebuffer notifier block */ struct notifier_block fb_notif;
- /* list entry of all registered backlight devices */
- struct list_head entry;
- struct device dev;
};
@@ -123,6 +126,7 @@ extern void devm_backlight_device_unregister(struct device *dev, struct backlight_device *bd); extern void backlight_force_update(struct backlight_device *bd, enum backlight_update_reason reason); +extern bool backlight_device_registered(enum backlight_type type);
#define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
Thanks!
Thanks for the review, -Aaron
On Thursday, October 10, 2013 08:54:29 AM Aaron Lu wrote:
On 10/10/2013 08:25 AM, Rafael J. Wysocki wrote:
On Tuesday, October 08, 2013 02:39:58 PM Aaron Lu wrote:
Introduce a new API for modules to query if a specific type of backlight device has been registered. This is useful for some backlight device provider module(e.g. ACPI video) to know if a native control interface(e.g. the interface created by i915) is available and then do things accordingly(e.g. avoid register its own on Win8 systems).
Signed-off-by: Aaron Lu aaron.lu@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com Tested-by: Yves-Alexis Perez corsac@debian.org Tested-by: Mika Westerberg mika.westerberg@linux.intel.com
drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++++ include/linux/backlight.h | 4 ++++ 2 files changed, 35 insertions(+)
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c index 94a403a..bf2d71d 100644 --- a/drivers/video/backlight/backlight.c +++ b/drivers/video/backlight/backlight.c @@ -21,6 +21,9 @@ #include <asm/backlight.h> #endif
+static struct list_head bd_list_head; +static struct mutex bd_list_mutex;
I'd prefer these two things to be called backlight_dev_list and backlight_dev_list_mutex, respectively.
OK.
static const char *const backlight_types[] = { [BACKLIGHT_RAW] = "raw", [BACKLIGHT_PLATFORM] = "platform", @@ -349,10 +352,32 @@ struct backlight_device *backlight_device_register(const char *name, mutex_unlock(&pmac_backlight_mutex); #endif
- mutex_lock(&bd_list_mutex);
- list_add(&new_bd->entry, &bd_list_head);
- mutex_unlock(&bd_list_mutex);
- return new_bd;
} EXPORT_SYMBOL(backlight_device_register);
+bool backlight_device_registered(enum backlight_type type) +{
- bool found = false;
- struct backlight_device *bd;
- mutex_lock(&bd_list_mutex);
- list_for_each_entry(bd, &bd_list_head, entry) {
if (bd->props.type == type) {found = true;break;}- }
Isn't it useful to be able to register more than one backlight device of the same type sometimes?
I think so for some kind of computers. OTOH, the above function should be enough for the problem we are solving here, if someday we need to differentiate, we can enhance the code then.
OK
On Thu, 10 Oct 2013, Aaron Lu aaron.lu@intel.com wrote:
On 10/10/2013 08:25 AM, Rafael J. Wysocki wrote:
On Tuesday, October 08, 2013 02:39:58 PM Aaron Lu wrote:
+bool backlight_device_registered(enum backlight_type type) +{
- bool found = false;
- struct backlight_device *bd;
- mutex_lock(&bd_list_mutex);
- list_for_each_entry(bd, &bd_list_head, entry) {
if (bd->props.type == type) {found = true;break;}- }
Isn't it useful to be able to register more than one backlight device of the same type sometimes?
I think so for some kind of computers. OTOH, the above function should be enough for the problem we are solving here, if someday we need to differentiate, we can enhance the code then.
Since both Baytrail and Haswell already have two backlight PWMs, this may be needed sooner than you think. But we shouldn't let that block fixing the more urgent issue we have now. So I'm fine with this. It doesn't prevent one from registering more than one device of the same type anyway.
BR, Jani.
On 10/10/2013 12:29 PM, Jani Nikula wrote:
On Thu, 10 Oct 2013, Aaron Lu aaron.lu@intel.com wrote:
On 10/10/2013 08:25 AM, Rafael J. Wysocki wrote:
On Tuesday, October 08, 2013 02:39:58 PM Aaron Lu wrote:
+bool backlight_device_registered(enum backlight_type type) +{
- bool found = false;
- struct backlight_device *bd;
- mutex_lock(&bd_list_mutex);
- list_for_each_entry(bd, &bd_list_head, entry) {
if (bd->props.type == type) {found = true;break;}- }
Isn't it useful to be able to register more than one backlight device of the same type sometimes?
I think so for some kind of computers. OTOH, the above function should be enough for the problem we are solving here, if someday we need to differentiate, we can enhance the code then.
Since both Baytrail and Haswell already have two backlight PWMs, this may be needed sooner than you think. But we shouldn't let that block
Do we need to differentiate which backlight PWM is registered to decide if ACPI video backlight interface should be skipped? My understanding is no.
Thanks, Aaron
fixing the more urgent issue we have now. So I'm fine with this. It doesn't prevent one from registering more than one device of the same type anyway.
BR, Jani.
On Thu, 10 Oct 2013, Aaron Lu aaron.lu@intel.com wrote:
On 10/10/2013 12:29 PM, Jani Nikula wrote:
On Thu, 10 Oct 2013, Aaron Lu aaron.lu@intel.com wrote:
On 10/10/2013 08:25 AM, Rafael J. Wysocki wrote:
On Tuesday, October 08, 2013 02:39:58 PM Aaron Lu wrote:
+bool backlight_device_registered(enum backlight_type type) +{
- bool found = false;
- struct backlight_device *bd;
- mutex_lock(&bd_list_mutex);
- list_for_each_entry(bd, &bd_list_head, entry) {
if (bd->props.type == type) {found = true;break;}- }
Isn't it useful to be able to register more than one backlight device of the same type sometimes?
I think so for some kind of computers. OTOH, the above function should be enough for the problem we are solving here, if someday we need to differentiate, we can enhance the code then.
Since both Baytrail and Haswell already have two backlight PWMs, this may be needed sooner than you think. But we shouldn't let that block
Do we need to differentiate which backlight PWM is registered to decide if ACPI video backlight interface should be skipped? My understanding is no.
That's correct. If things change, we can fix it then.
Jani.
Thanks, Aaron
fixing the more urgent issue we have now. So I'm fine with this. It doesn't prevent one from registering more than one device of the same type anyway.
BR, Jani.
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.
APIs to selectively remove backlight control interface and/or event delivery functionality can be easily added once needed.
Signed-off-by: Aaron Lu aaron.lu@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com Tested-by: Yves-Alexis Perez corsac@debian.org Tested-by: Mika Westerberg mika.westerberg@linux.intel.com --- drivers/acpi/video.c | 434 +++++++++++++++++++++++++++++---------------------- 1 file changed, 245 insertions(+), 189 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index aebcf63..3bd1eaa 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,236 @@ 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; +} + +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 +1837,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 +1871,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 +1927,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;
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".
So for Win8 systems, if there is native backlight control interface registered by GPU driver, ACPI video will not register its own. For users who prefer to keep ACPI video's backlight interface, the existing kernel cmdline option acpi_backlight=video can be used.
Signed-off-by: Aaron Lu aaron.lu@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com Tested-by: Yves-Alexis Perez corsac@debian.org Tested-by: Mika Westerberg mika.westerberg@linux.intel.com --- drivers/acpi/internal.h | 5 ++--- drivers/acpi/video.c | 10 +++++----- drivers/acpi/video_detect.c | 14 ++++++++++++-- 3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 20f4233..453ae8d 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -169,9 +169,8 @@ int acpi_create_platform_device(struct acpi_device *adev, Video -------------------------------------------------------------------------- */ #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) -bool acpi_video_backlight_quirks(void); -#else -static inline bool acpi_video_backlight_quirks(void) { return false; } +bool acpi_osi_is_win8(void); +bool acpi_video_verify_backlight_support(void); #endif
#endif /* _ACPI_INTERNAL_H_ */ diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 3bd1eaa..343db59 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) @@ -1386,13 +1386,13 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video, static int acpi_video_bus_start_devices(struct acpi_video_bus *video) { return acpi_video_bus_DOS(video, 0, - acpi_video_backlight_quirks() ? 1 : 0); + acpi_osi_is_win8() ? 1 : 0); }
static int acpi_video_bus_stop_devices(struct acpi_video_bus *video) { return acpi_video_bus_DOS(video, 0, - acpi_video_backlight_quirks() ? 0 : 1); + acpi_osi_is_win8() ? 0 : 1); }
static void acpi_video_bus_notify(struct acpi_device *device, u32 event) @@ -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; diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 940edbf..23d7d26 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -37,6 +37,7 @@ #include <linux/acpi.h> #include <linux/dmi.h> #include <linux/pci.h> +#include <linux/backlight.h>
#include "internal.h"
@@ -233,11 +234,11 @@ static void acpi_video_caps_check(void) acpi_video_get_capabilities(NULL); }
-bool acpi_video_backlight_quirks(void) +bool acpi_osi_is_win8(void) { return acpi_gbl_osi_data >= ACPI_OSI_WIN_8; } -EXPORT_SYMBOL(acpi_video_backlight_quirks); +EXPORT_SYMBOL(acpi_osi_is_win8);
/* Promote the vendor interface instead of the generic video module. * This function allow DMI blacklists to be implemented by externals @@ -283,6 +284,15 @@ int acpi_video_backlight_support(void) } EXPORT_SYMBOL(acpi_video_backlight_support);
+bool acpi_video_verify_backlight_support(void) +{ + if (!(acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO) && + acpi_osi_is_win8() && backlight_device_registered(BACKLIGHT_RAW)) + return false; + return 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.
On Tuesday, October 08, 2013 02:40:00 PM 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".
So for Win8 systems, if there is native backlight control interface registered by GPU driver, ACPI video will not register its own. For users who prefer to keep ACPI video's backlight interface, the existing kernel cmdline option acpi_backlight=video can be used.
Signed-off-by: Aaron Lu aaron.lu@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com Tested-by: Yves-Alexis Perez corsac@debian.org Tested-by: Mika Westerberg mika.westerberg@linux.intel.com
drivers/acpi/internal.h | 5 ++--- drivers/acpi/video.c | 10 +++++----- drivers/acpi/video_detect.c | 14 ++++++++++++-- 3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 20f4233..453ae8d 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -169,9 +169,8 @@ int acpi_create_platform_device(struct acpi_device *adev, Video -------------------------------------------------------------------------- */ #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) -bool acpi_video_backlight_quirks(void); -#else -static inline bool acpi_video_backlight_quirks(void) { return false; } +bool acpi_osi_is_win8(void); +bool acpi_video_verify_backlight_support(void); #endif
#endif /* _ACPI_INTERNAL_H_ */ diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 3bd1eaa..343db59 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)
@@ -1386,13 +1386,13 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video, static int acpi_video_bus_start_devices(struct acpi_video_bus *video) { return acpi_video_bus_DOS(video, 0,
acpi_video_backlight_quirks() ? 1 : 0);
acpi_osi_is_win8() ? 1 : 0);}
static int acpi_video_bus_stop_devices(struct acpi_video_bus *video) { return acpi_video_bus_DOS(video, 0,
acpi_video_backlight_quirks() ? 0 : 1);
acpi_osi_is_win8() ? 0 : 1);}
static void acpi_video_bus_notify(struct acpi_device *device, u32 event) @@ -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;
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 940edbf..23d7d26 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -37,6 +37,7 @@ #include <linux/acpi.h> #include <linux/dmi.h> #include <linux/pci.h> +#include <linux/backlight.h>
#include "internal.h"
@@ -233,11 +234,11 @@ static void acpi_video_caps_check(void) acpi_video_get_capabilities(NULL); }
-bool acpi_video_backlight_quirks(void) +bool acpi_osi_is_win8(void) { return acpi_gbl_osi_data >= ACPI_OSI_WIN_8; } -EXPORT_SYMBOL(acpi_video_backlight_quirks); +EXPORT_SYMBOL(acpi_osi_is_win8);
/* Promote the vendor interface instead of the generic video module.
- This function allow DMI blacklists to be implemented by externals
@@ -283,6 +284,15 @@ int acpi_video_backlight_support(void) } EXPORT_SYMBOL(acpi_video_backlight_support);
+bool acpi_video_verify_backlight_support(void) +{
- if (!(acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO) &&
acpi_osi_is_win8() && backlight_device_registered(BACKLIGHT_RAW))return false;
If I'm not mistaken, this will introduce a regression for the people who have problems with the native i915 backlight on Win8-compatible systems. I'd prefer to avoid that at this point.
- return 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.
Thanks!
On 10/10/2013 08:29 AM, Rafael J. Wysocki wrote:
On Tuesday, October 08, 2013 02:40:00 PM 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".
So for Win8 systems, if there is native backlight control interface registered by GPU driver, ACPI video will not register its own. For users who prefer to keep ACPI video's backlight interface, the existing kernel cmdline option acpi_backlight=video can be used.
Signed-off-by: Aaron Lu aaron.lu@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com Tested-by: Yves-Alexis Perez corsac@debian.org Tested-by: Mika Westerberg mika.westerberg@linux.intel.com
drivers/acpi/internal.h | 5 ++--- drivers/acpi/video.c | 10 +++++----- drivers/acpi/video_detect.c | 14 ++++++++++++-- 3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 20f4233..453ae8d 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -169,9 +169,8 @@ int acpi_create_platform_device(struct acpi_device *adev, Video -------------------------------------------------------------------------- */ #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) -bool acpi_video_backlight_quirks(void); -#else -static inline bool acpi_video_backlight_quirks(void) { return false; } +bool acpi_osi_is_win8(void); +bool acpi_video_verify_backlight_support(void); #endif
#endif /* _ACPI_INTERNAL_H_ */ diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 3bd1eaa..343db59 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)
@@ -1386,13 +1386,13 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video, static int acpi_video_bus_start_devices(struct acpi_video_bus *video) { return acpi_video_bus_DOS(video, 0,
acpi_video_backlight_quirks() ? 1 : 0);
acpi_osi_is_win8() ? 1 : 0);}
static int acpi_video_bus_stop_devices(struct acpi_video_bus *video) { return acpi_video_bus_DOS(video, 0,
acpi_video_backlight_quirks() ? 0 : 1);
acpi_osi_is_win8() ? 0 : 1);}
static void acpi_video_bus_notify(struct acpi_device *device, u32 event) @@ -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;
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 940edbf..23d7d26 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -37,6 +37,7 @@ #include <linux/acpi.h> #include <linux/dmi.h> #include <linux/pci.h> +#include <linux/backlight.h>
#include "internal.h"
@@ -233,11 +234,11 @@ static void acpi_video_caps_check(void) acpi_video_get_capabilities(NULL); }
-bool acpi_video_backlight_quirks(void) +bool acpi_osi_is_win8(void) { return acpi_gbl_osi_data >= ACPI_OSI_WIN_8; } -EXPORT_SYMBOL(acpi_video_backlight_quirks); +EXPORT_SYMBOL(acpi_osi_is_win8);
/* Promote the vendor interface instead of the generic video module.
- This function allow DMI blacklists to be implemented by externals
@@ -283,6 +284,15 @@ int acpi_video_backlight_support(void) } EXPORT_SYMBOL(acpi_video_backlight_support);
+bool acpi_video_verify_backlight_support(void) +{
- if (!(acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO) &&
acpi_osi_is_win8() && backlight_device_registered(BACKLIGHT_RAW))return false;If I'm not mistaken, this will introduce a regression for the people who have problems with the native i915 backlight on Win8-compatible systems. I'd prefer to avoid that at this point.
OK, I see.
Then I'm afraid a new kernel command line option is needed, something like video.use_native_backlight and set it to false by default, then for people who need to avoid the ACPI video backlight interface, they can add video.use_native_backlight=true to kernel cmdline.
One thing I need to mention is, with the new cmdline option, users will need to manually add a kernel cmdline option to make backlight work on their systems, while they can already make backlight work by modifying xorg.conf to specify using intel_backlight interface, so it doesn't seem this patchset will be very useful then...
Thanks, Aaron
- return 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.
Thanks!
On Thursday, October 10, 2013 09:02:55 AM Aaron Lu wrote:
On 10/10/2013 08:29 AM, Rafael J. Wysocki wrote:
On Tuesday, October 08, 2013 02:40:00 PM 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".
So for Win8 systems, if there is native backlight control interface registered by GPU driver, ACPI video will not register its own. For users who prefer to keep ACPI video's backlight interface, the existing kernel cmdline option acpi_backlight=video can be used.
Signed-off-by: Aaron Lu aaron.lu@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com Tested-by: Yves-Alexis Perez corsac@debian.org Tested-by: Mika Westerberg mika.westerberg@linux.intel.com
drivers/acpi/internal.h | 5 ++--- drivers/acpi/video.c | 10 +++++----- drivers/acpi/video_detect.c | 14 ++++++++++++-- 3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 20f4233..453ae8d 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -169,9 +169,8 @@ int acpi_create_platform_device(struct acpi_device *adev, Video -------------------------------------------------------------------------- */ #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) -bool acpi_video_backlight_quirks(void); -#else -static inline bool acpi_video_backlight_quirks(void) { return false; } +bool acpi_osi_is_win8(void); +bool acpi_video_verify_backlight_support(void); #endif
#endif /* _ACPI_INTERNAL_H_ */ diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 3bd1eaa..343db59 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)
@@ -1386,13 +1386,13 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video, static int acpi_video_bus_start_devices(struct acpi_video_bus *video) { return acpi_video_bus_DOS(video, 0,
acpi_video_backlight_quirks() ? 1 : 0);
acpi_osi_is_win8() ? 1 : 0);}
static int acpi_video_bus_stop_devices(struct acpi_video_bus *video) { return acpi_video_bus_DOS(video, 0,
acpi_video_backlight_quirks() ? 0 : 1);
acpi_osi_is_win8() ? 0 : 1);}
static void acpi_video_bus_notify(struct acpi_device *device, u32 event) @@ -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;
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 940edbf..23d7d26 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -37,6 +37,7 @@ #include <linux/acpi.h> #include <linux/dmi.h> #include <linux/pci.h> +#include <linux/backlight.h>
#include "internal.h"
@@ -233,11 +234,11 @@ static void acpi_video_caps_check(void) acpi_video_get_capabilities(NULL); }
-bool acpi_video_backlight_quirks(void) +bool acpi_osi_is_win8(void) { return acpi_gbl_osi_data >= ACPI_OSI_WIN_8; } -EXPORT_SYMBOL(acpi_video_backlight_quirks); +EXPORT_SYMBOL(acpi_osi_is_win8);
/* Promote the vendor interface instead of the generic video module.
- This function allow DMI blacklists to be implemented by externals
@@ -283,6 +284,15 @@ int acpi_video_backlight_support(void) } EXPORT_SYMBOL(acpi_video_backlight_support);
+bool acpi_video_verify_backlight_support(void) +{
- if (!(acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO) &&
acpi_osi_is_win8() && backlight_device_registered(BACKLIGHT_RAW))return false;If I'm not mistaken, this will introduce a regression for the people who have problems with the native i915 backlight on Win8-compatible systems. I'd prefer to avoid that at this point.
OK, I see.
Then I'm afraid a new kernel command line option is needed, something like video.use_native_backlight and set it to false by default, then for people who need to avoid the ACPI video backlight interface, they can add video.use_native_backlight=true to kernel cmdline.
One thing I need to mention is, with the new cmdline option, users will need to manually add a kernel cmdline option to make backlight work on their systems, while they can already make backlight work by modifying xorg.conf to specify using intel_backlight interface, so it doesn't seem this patchset will be very useful then...
Except if we add a (black)list of systems where that option will be 'true' by default instead of the _OSI blacklist we have today.
Also we can switch the default during development cycles to get an idea about how many systems are affected and maybe we can find a way to fix them, in which case we can simply drop the option.
Thanks, Rafael
On 10/10/2013 08:59 PM, Rafael J. Wysocki wrote:
On Thursday, October 10, 2013 09:02:55 AM Aaron Lu wrote:
On 10/10/2013 08:29 AM, Rafael J. Wysocki wrote:
On Tuesday, October 08, 2013 02:40:00 PM 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".
So for Win8 systems, if there is native backlight control interface registered by GPU driver, ACPI video will not register its own. For users who prefer to keep ACPI video's backlight interface, the existing kernel cmdline option acpi_backlight=video can be used.
Signed-off-by: Aaron Lu aaron.lu@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com Tested-by: Yves-Alexis Perez corsac@debian.org Tested-by: Mika Westerberg mika.westerberg@linux.intel.com
drivers/acpi/internal.h | 5 ++--- drivers/acpi/video.c | 10 +++++----- drivers/acpi/video_detect.c | 14 ++++++++++++-- 3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 20f4233..453ae8d 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -169,9 +169,8 @@ int acpi_create_platform_device(struct acpi_device *adev, Video -------------------------------------------------------------------------- */ #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) -bool acpi_video_backlight_quirks(void); -#else -static inline bool acpi_video_backlight_quirks(void) { return false; } +bool acpi_osi_is_win8(void); +bool acpi_video_verify_backlight_support(void); #endif
#endif /* _ACPI_INTERNAL_H_ */ diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index 3bd1eaa..343db59 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)
@@ -1386,13 +1386,13 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video, static int acpi_video_bus_start_devices(struct acpi_video_bus *video) { return acpi_video_bus_DOS(video, 0,
acpi_video_backlight_quirks() ? 1 : 0);
acpi_osi_is_win8() ? 1 : 0);}
static int acpi_video_bus_stop_devices(struct acpi_video_bus *video) { return acpi_video_bus_DOS(video, 0,
acpi_video_backlight_quirks() ? 0 : 1);
acpi_osi_is_win8() ? 0 : 1);}
static void acpi_video_bus_notify(struct acpi_device *device, u32 event) @@ -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;
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 940edbf..23d7d26 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -37,6 +37,7 @@ #include <linux/acpi.h> #include <linux/dmi.h> #include <linux/pci.h> +#include <linux/backlight.h>
#include "internal.h"
@@ -233,11 +234,11 @@ static void acpi_video_caps_check(void) acpi_video_get_capabilities(NULL); }
-bool acpi_video_backlight_quirks(void) +bool acpi_osi_is_win8(void) { return acpi_gbl_osi_data >= ACPI_OSI_WIN_8; } -EXPORT_SYMBOL(acpi_video_backlight_quirks); +EXPORT_SYMBOL(acpi_osi_is_win8);
/* Promote the vendor interface instead of the generic video module.
- This function allow DMI blacklists to be implemented by externals
@@ -283,6 +284,15 @@ int acpi_video_backlight_support(void) } EXPORT_SYMBOL(acpi_video_backlight_support);
+bool acpi_video_verify_backlight_support(void) +{
- if (!(acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO) &&
acpi_osi_is_win8() && backlight_device_registered(BACKLIGHT_RAW))return false;If I'm not mistaken, this will introduce a regression for the people who have problems with the native i915 backlight on Win8-compatible systems. I'd prefer to avoid that at this point.
OK, I see.
Then I'm afraid a new kernel command line option is needed, something like video.use_native_backlight and set it to false by default, then for people who need to avoid the ACPI video backlight interface, they can add video.use_native_backlight=true to kernel cmdline.
One thing I need to mention is, with the new cmdline option, users will need to manually add a kernel cmdline option to make backlight work on their systems, while they can already make backlight work by modifying xorg.conf to specify using intel_backlight interface, so it doesn't seem this patchset will be very useful then...
Except if we add a (black)list of systems where that option will be 'true' by default instead of the _OSI blacklist we have today.
Also we can switch the default during development cycles to get an idea about how many systems are affected and maybe we can find a way to fix them, in which case we can simply drop the option.
Sounds good, I'll update in next revision, thanks for the suggestion!
-Aaron
The tpacpi_acpi_handle_locate function makes use of acpi_get_devices to locate handle for ACPI video by HID, the problem is, ACPI video node doesn't really have HID defined(i.e. no _HID control method is defined for video device), so.. that function would fail. This can be solved by enhancing the callback function for acpi_get_devices, where we can use acpi_device_hid function to check if the ACPI node corresponds to a video controller.
In addition to that, the _BCL control method only exists under a video output device node, not a video controller device node. So to evaluate _BCL, we need the handle of a video output device node, which is child of the located video controller node from tpacpi_acpi_handle_locate.
The two fix are necessary for some Thinkpad models to emit notification on backlight hotkey press as a result of evaluation of _BCL.
Signed-off-by: Aaron Lu aaron.lu@intel.com Tested-by: Igor Gnatenko i.gnatenko.brain@gmail.com Acked-by: Henrique de Moraes Holschuh hmh@hmh.eng.br --- drivers/platform/x86/thinkpad_acpi.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 03ca6c1..170f278 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -700,6 +700,14 @@ static void __init drv_acpi_handle_init(const char *name, static acpi_status __init tpacpi_acpi_handle_locate_callback(acpi_handle handle, u32 level, void *context, void **return_value) { + struct acpi_device *dev; + if (!strcmp(context, "video")) { + if (acpi_bus_get_device(handle, &dev)) + return AE_OK; + if (strcmp(ACPI_VIDEO_HID, acpi_device_hid(dev))) + return AE_OK; + } + *(acpi_handle *)return_value = handle;
return AE_CTRL_TERMINATE; @@ -712,10 +720,10 @@ static void __init tpacpi_acpi_handle_locate(const char *name, acpi_status status; acpi_handle device_found;
- BUG_ON(!name || !hid || !handle); + BUG_ON(!name || !handle); vdbg_printk(TPACPI_DBG_INIT, "trying to locate ACPI handle for %s, using HID %s\n", - name, hid); + name, hid ? hid : "NULL");
memset(&device_found, 0, sizeof(device_found)); status = acpi_get_devices(hid, tpacpi_acpi_handle_locate_callback, @@ -6090,19 +6098,28 @@ static int __init tpacpi_query_bcl_levels(acpi_handle handle) { struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; union acpi_object *obj; + struct acpi_device *device, *child; int rc;
- if (ACPI_SUCCESS(acpi_evaluate_object(handle, "_BCL", NULL, &buffer))) { + if (acpi_bus_get_device(handle, &device)) + return 0; + + rc = 0; + list_for_each_entry(child, &device->children, node) { + acpi_status status = acpi_evaluate_object(child->handle, "_BCL", + NULL, &buffer); + if (ACPI_FAILURE(status)) + continue; + obj = (union acpi_object *)buffer.pointer; if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) { pr_err("Unknown _BCL data, please report this to %s\n", - TPACPI_MAIL); + TPACPI_MAIL); rc = 0; } else { rc = obj->package.count; } - } else { - return 0; + break; }
kfree(buffer.pointer); @@ -6118,7 +6135,7 @@ static unsigned int __init tpacpi_check_std_acpi_brightness_support(void) acpi_handle video_device; int bcl_levels = 0;
- tpacpi_acpi_handle_locate("video", ACPI_VIDEO_HID, &video_device); + tpacpi_acpi_handle_locate("video", NULL, &video_device); if (video_device) bcl_levels = tpacpi_query_bcl_levels(video_device);
dri-devel@lists.freedesktop.org
-
Aaron Lu -
Jani Nikula -
Rafael J. Wysocki -
Rafael J. Wysocki