LinuxLists.cc - [PATCH v2 0/3] Fix Win8 backlight issue

[permalink] [raw]

Subject: [PATCH v2 2/3] ACPI / video: seperate backlight control and event interface

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 <[email protected]>
---
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)
{
--
1.8.4.12.g2ea3df6

2013-09-17 09:23:08

[permalink] [raw]

Subject: [PATCH v2 1/3] backlight: introduce backlight_device_registered

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 <[email protected]>
---
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)

--
1.8.4.12.g2ea3df6

2013-09-17 09:23:36

[permalink] [raw]

Subject: [PATCH v2 3/3] ACPI / video: Do not register backlight if win8 and native interface exists

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.

This patch is an evolution from previous work done by Matthew Garrett,
Chun-Yi Lee, Seth Forshee and Rafael J. Wysocki.

Signed-off-by: Aaron Lu <[email protected]>
---
drivers/acpi/internal.h | 5 ++---
drivers/acpi/video.c | 27 +++++----------------------
drivers/acpi/video_detect.c | 14 ++++++++++++--
3 files changed, 19 insertions(+), 27 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 5ad5a71..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;
@@ -1677,23 +1677,6 @@ static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video)
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;
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.
--
1.8.4.12.g2ea3df6

2013-09-17 13:03:48

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] backlight: introduce backlight_device_registered

On Tue, 2013-09-17 at 17:23 +0800, 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 <[email protected]>
Tested-by: Igor Gnatenko <[email protected]>
> ---
> drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++++
> include/linux/backlight.h | 4 ++++
> 2 files changed, 35 insertions(+)

--
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.12.0-0.rc1.git0.1.fc20.x86_64

2013-09-17 13:04:21

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ACPI / video: seperate backlight control and event interface

On Tue, 2013-09-17 at 17:23 +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.
>
> APIs to selectively remove backlight control interface and/or event
> delivery functionality can be easily added once needed.
>
> Signed-off-by: Aaron Lu <[email protected]>
Tested-by: Igor Gnatenko <[email protected]>
> ---
> drivers/acpi/video.c | 451 ++++++++++++++++++++++++++++++---------------------
> include/acpi/video.h | 2 +
> 2 files changed, 264 insertions(+), 189 deletions(-)

--
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.12.0-0.rc1.git0.1.fc20.x86_64

2013-09-17 13:04:46

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] ACPI / video: Do not register backlight if win8 and native interface exists

On Tue, 2013-09-17 at 17:23 +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".
>
> 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.
>
> This patch is an evolution from previous work done by Matthew Garrett,
> Chun-Yi Lee, Seth Forshee and Rafael J. Wysocki.
>
> Signed-off-by: Aaron Lu <[email protected]>
Tested-by: Igor Gnatenko <[email protected]>
> ---
> drivers/acpi/internal.h | 5 ++---
> drivers/acpi/video.c | 27 +++++----------------------
> drivers/acpi/video_detect.c | 14 ++++++++++++--
> 3 files changed, 19 insertions(+), 27 deletions(-)

--
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.12.0-0.rc1.git0.1.fc20.x86_64

2013-09-17 13:34:20

[permalink] [raw]

Subject: Re: [PATCH v2 0/3] Fix Win8 backlight issue

On Tue, 2013-09-17 at 17:23 +0800, Aaron Lu wrote:
> 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 three 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/3 for details.
> Then patch 2/3 does the cleanup to sepeate the backlight control and
> event delivery functionality in the ACPI video module and patch 3/3
> 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.
>
> Technically, patch 2/3 is not required to fix the issue here. So if you
> think it is not necessary, I can remove it from the series.
>
> Apply on top of v3.12-rc1.
>
> Aaron Lu (3):
> 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
>
> drivers/acpi/internal.h | 5 +-
> drivers/acpi/video.c | 442 ++++++++++++++++++++----------------
> drivers/acpi/video_detect.c | 14 +-
> drivers/video/backlight/backlight.c | 31 +++
> include/acpi/video.h | 2 +
> include/linux/backlight.h | 4 +
> 6 files changed, 300 insertions(+), 198 deletions(-)
>

Aaron, how about fix indicator on ThinkPads ?

--
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.1-300.fc20.x86_64

2013-09-18 01:03:17

[permalink] [raw]

Subject: Re: [PATCH v2 0/3] Fix Win8 backlight issue

On 09/17/2013 09:34 PM, Igor Gnatenko wrote:
> On Tue, 2013-09-17 at 17:23 +0800, Aaron Lu wrote:
>> 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 three 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/3 for details.
>> Then patch 2/3 does the cleanup to sepeate the backlight control and
>> event delivery functionality in the ACPI video module and patch 3/3
>> 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.
>>
>> Technically, patch 2/3 is not required to fix the issue here. So if you
>> think it is not necessary, I can remove it from the series.
>>
>> Apply on top of v3.12-rc1.
>>
>> Aaron Lu (3):
>> 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
>>
>> drivers/acpi/internal.h | 5 +-
>> drivers/acpi/video.c | 442 ++++++++++++++++++++----------------
>> drivers/acpi/video_detect.c | 14 +-
>> drivers/video/backlight/backlight.c | 31 +++
>> include/acpi/video.h | 2 +
>> include/linux/backlight.h | 4 +
>> 6 files changed, 300 insertions(+), 198 deletions(-)
>>
>
> Aaron, how about fix indicator on ThinkPads ?

Can you please describe the problem in detail, is it that when you
adjust brightness level through hotkey, there is no GUI indication?
Thanks.

-Aaron

2013-09-18 06:30:26

[permalink] [raw]

Subject: Re: [PATCH v2 0/3] Fix Win8 backlight issue

On Wed, 2013-09-18 at 09:03 +0800, Aaron Lu wrote:
> On 09/17/2013 09:34 PM, Igor Gnatenko wrote:
> > On Tue, 2013-09-17 at 17:23 +0800, Aaron Lu wrote:
> >> 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 three 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/3 for details.
> >> Then patch 2/3 does the cleanup to sepeate the backlight control and
> >> event delivery functionality in the ACPI video module and patch 3/3
> >> 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.
> >>
> >> Technically, patch 2/3 is not required to fix the issue here. So if you
> >> think it is not necessary, I can remove it from the series.
> >>
> >> Apply on top of v3.12-rc1.
> >>
> >> Aaron Lu (3):
> >> 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
> >>
> >> drivers/acpi/internal.h | 5 +-
> >> drivers/acpi/video.c | 442 ++++++++++++++++++++----------------
> >> drivers/acpi/video_detect.c | 14 +-
> >> drivers/video/backlight/backlight.c | 31 +++
> >> include/acpi/video.h | 2 +
> >> include/linux/backlight.h | 4 +
> >> 6 files changed, 300 insertions(+), 198 deletions(-)
> >>
> >
> > Aaron, how about fix indicator on ThinkPads ?
>
> Can you please describe the problem in detail, is it that when you
> adjust brightness level through hotkey, there is no GUI indication?
> Thanks.
>
> -Aaron

Yes. On my ThinkPad X230 I pressing backlight hotkeys. Actually
brightnes changing, but have no indicator in GUI.

--
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.1-300.fc20.x86_64

2013-09-18 12:30:32

[permalink] [raw]

Subject: Re: [PATCH v2 0/3] Fix Win8 backlight issue

On 09/18/2013 02:30 PM, Igor Gnatenko wrote:
> On Wed, 2013-09-18 at 09:03 +0800, Aaron Lu wrote:
>> On 09/17/2013 09:34 PM, Igor Gnatenko wrote:
>>>
>>> Aaron, how about fix indicator on ThinkPads ?
>>
>> Can you please describe the problem in detail, is it that when you
>> adjust brightness level through hotkey, there is no GUI indication?
>> Thanks.
>>
>> -Aaron
>
> Yes. On my ThinkPad X230 I pressing backlight hotkeys. Actually
> brightnes changing, but have no indicator in GUI.

Oh, that's still the problem of _BCL not getting executed once for
Lenovo thinkpad laptops. I borrowed a Thinkpad X1 this afternoon and can
reproduce this, I'll take a look at this issue. The thinkpad-acpi module
already has a call to _BCL but somehow that doesn't happen.

Since it's national holidays here, I'll check this issue when I got back
to work on this Saturday. Thanks for the quick test :-)

-Aaron

2013-09-18 12:36:23

[permalink] [raw]

Subject: Re: [PATCH v2 0/3] Fix Win8 backlight issue

On Wed, 2013-09-18 at 20:31 +0800, Aaron Lu wrote:
> On 09/18/2013 02:30 PM, Igor Gnatenko wrote:
> > On Wed, 2013-09-18 at 09:03 +0800, Aaron Lu wrote:
> >> On 09/17/2013 09:34 PM, Igor Gnatenko wrote:
> >>>
> >>> Aaron, how about fix indicator on ThinkPads ?
> >>
> >> Can you please describe the problem in detail, is it that when you
> >> adjust brightness level through hotkey, there is no GUI indication?
> >> Thanks.
> >>
> >> -Aaron
> >
> > Yes. On my ThinkPad X230 I pressing backlight hotkeys. Actually
> > brightnes changing, but have no indicator in GUI.
>
> Oh, that's still the problem of _BCL not getting executed once for
> Lenovo thinkpad laptops. I borrowed a Thinkpad X1 this afternoon and can
> reproduce this, I'll take a look at this issue. The thinkpad-acpi module
> already has a call to _BCL but somehow that doesn't happen.
>
> Since it's national holidays here, I'll check this issue when I got back
> to work on this Saturday. Thanks for the quick test :-)
Thanks. No problem ;-)
>
> -Aaron

--
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.1-300.fc20.x86_64

2013-09-20 08:34:43

by Jani Nikula

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] ACPI / video: Do not register backlight if win8 and native interface exists

On Tue, 17 Sep 2013, Aaron Lu <[email protected]> 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.
>
> This patch is an evolution from previous work done by Matthew Garrett,
> Chun-Yi Lee, Seth Forshee and Rafael J. Wysocki.
>
> Signed-off-by: Aaron Lu <[email protected]>
> ---
> drivers/acpi/internal.h | 5 ++---
> drivers/acpi/video.c | 27 +++++----------------------
> drivers/acpi/video_detect.c | 14 ++++++++++++--
> 3 files changed, 19 insertions(+), 27 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 5ad5a71..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;
> @@ -1677,23 +1677,6 @@ static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video)
> 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);

You add this in patch 2/3 only to remove it again in 3/3?

> -
> static void acpi_video_dev_add_notify_handler(struct acpi_video_device *device)
> {
> acpi_status status;
> 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();
> +}

IIUC this expects the raw backlight device to have been registered prior
to calling acpi_video_register(). This only works for i915 which calls
acpi_video_register() itself.

BR,
Jani.

> +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.
> --
> 1.8.4.12.g2ea3df6
>

--
Jani Nikula, Intel Open Source Technology Center

2013-09-20 13:01:35

[permalink] [raw]

Subject: Re: [PATCH v2 0/3] Fix Win8 backlight issue

On mar., 2013-09-17 at 17:23 +0800, Aaron Lu wrote:
> 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 three 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/3 for
> details.
> Then patch 2/3 does the cleanup to sepeate the backlight control and
> event delivery functionality in the ACPI video module and patch 3/3
> 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.

I've tested this on my x230 (using pure UEFI with CSM disabled). As far
as I can tell, it works as I would expect:

- brightness keys work in initramfs, in console after boot, in lightdm
prompt and in Xfce (wether or not xfce4-power-manager is running).

I don't have brightness notifications (from xfce4-power-manager) but I
don't usually need them so I'm fine with that.

Regards,
--
Yves-Alexis

Attachments:

signature.asc (490.00 B)
This is a digitally signed message part

2013-09-20 13:02:24

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] backlight: introduce backlight_device_registered

On mar., 2013-09-17 at 17:23 +0800, 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 <[email protected]>
Tested-by: Yves-Alexis Perez <[email protected]>
--
Yves-Alexis

Attachments:

signature.asc (490.00 B)
This is a digitally signed message part

2013-09-20 13:02:43

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] ACPI / video: seperate backlight control and event interface

On mar., 2013-09-17 at 17:23 +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.
>
> APIs to selectively remove backlight control interface and/or event
> delivery functionality can be easily added once needed.
>
> Signed-off-by: Aaron Lu <[email protected]>
Tested-by: Yves-Alexis Perez <[email protected]>
--
Yves-Alexis

Attachments:

signature.asc (490.00 B)
This is a digitally signed message part

2013-09-20 13:03:20

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] ACPI / video: Do not register backlight if win8 and native interface exists

On mar., 2013-09-17 at 17:23 +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".
>
> 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.
>
> This patch is an evolution from previous work done by Matthew Garrett,
> Chun-Yi Lee, Seth Forshee and Rafael J. Wysocki.
>
> Signed-off-by: Aaron Lu <[email protected]>
Tested-by: Yves-Alexis Perez <[email protected]>
--
Yves-Alexis

Attachments:

signature.asc (490.00 B)
This is a digitally signed message part

2013-09-22 02:46:35

[permalink] [raw]

Subject: Re: [PATCH v2 3/3] ACPI / video: Do not register backlight if win8 and native interface exists

On 09/20/2013 04:36 PM, Jani Nikula wrote:
> On Tue, 17 Sep 2013, Aaron Lu <[email protected]> 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.
>>
>> This patch is an evolution from previous work done by Matthew Garrett,
>> Chun-Yi Lee, Seth Forshee and Rafael J. Wysocki.
>>
>> Signed-off-by: Aaron Lu <[email protected]>
>> ---
>> drivers/acpi/internal.h | 5 ++---
>> drivers/acpi/video.c | 27 +++++----------------------
>> drivers/acpi/video_detect.c | 14 ++++++++++++--
>> 3 files changed, 19 insertions(+), 27 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 5ad5a71..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;
>> @@ -1677,23 +1677,6 @@ static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video)
>> 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);
>
> You add this in patch 2/3 only to remove it again in 3/3?

This is a mistake while I'm preparing the patches, I'll fix it in next
revision, thanks for pointing this out.

>
>> -
>> static void acpi_video_dev_add_notify_handler(struct acpi_video_device *device)
>> {
>> acpi_status status;
>> 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();
>> +}
>
> IIUC this expects the raw backlight device to have been registered prior
> to calling acpi_video_register(). This only works for i915 which calls
> acpi_video_register() itself.

Yes. Since all problematic laptops reported till now are intel graphics
card based, I didn't cover other cases.

Thanks,
Aaron

>
>
> BR,
> Jani.
>
>
>
>> +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.
>> --
>> 1.8.4.12.g2ea3df6
>>
>

2013-09-22 09:10:31

[permalink] [raw]

Subject: Re: [PATCH v2 0/3] Fix Win8 backlight issue

On 09/18/2013 08:36 PM, Igor Gnatenko wrote:
> On Wed, 2013-09-18 at 20:31 +0800, Aaron Lu wrote:
>> On 09/18/2013 02:30 PM, Igor Gnatenko wrote:
>>> On Wed, 2013-09-18 at 09:03 +0800, Aaron Lu wrote:
>>>> On 09/17/2013 09:34 PM, Igor Gnatenko wrote:
>>>>>
>>>>> Aaron, how about fix indicator on ThinkPads ?
>>>>
>>>> Can you please describe the problem in detail, is it that when you
>>>> adjust brightness level through hotkey, there is no GUI indication?
>>>> Thanks.
>>>>
>>>> -Aaron
>>>
>>> Yes. On my ThinkPad X230 I pressing backlight hotkeys. Actually
>>> brightnes changing, but have no indicator in GUI.
>>
>> Oh, that's still the problem of _BCL not getting executed once for
>> Lenovo thinkpad laptops. I borrowed a Thinkpad X1 this afternoon and can
>> reproduce this, I'll take a look at this issue. The thinkpad-acpi module
>> already has a call to _BCL but somehow that doesn't happen.
>>
>> Since it's national holidays here, I'll check this issue when I got back
>> to work on this Saturday. Thanks for the quick test :-)
> Thanks. No problem ;-)

Here is a quick fix for thinkpad-acpi.c:
https://github.com/aaronlu/linux acpi_video_win8
commit thinkpad-acpi: fix handle locate for video and query of _BCL.

Note that it is a separate issue specifically for thinkpad so I'll
submit that patch in another thread.

Thanks,
Aaron

2013-09-22 10:23:32