Fix Win8 backlight issue

---
 drivers/video/backlight/backlight.c | 31 +++++++++++++++++++++++++++++++
 include/linux/backlight.h           |  4 ++++
 2 files changed, 35 insertions(+)

drivers/video/backlight/backlight.c +31 -0

index 94a403a..bf2d71d 100644
@@ -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;
 }

include/linux/backlight.h +4 -0

index 53b7794..5f9cd96 100644
@@ -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)

Signature · 1 line

1.8.4.12.g2ea3df6

Aaron Lu

author

2013-09-24 09:47 UTC #2 lore raw

PATCH v3 2/4 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 | 434 +++++++++++++++++++++++++++++----------------------
 include/acpi/video.h |   2 +
 2 files changed, 247 insertions(+), 189 deletions(-)

drivers/acpi/video.c +245 ~~-189~~

index aebcf63..3bd1eaa 100644
@@ -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;

include/acpi/video.h +2 -0

index 61109f2..0c665b5 100644
@@ -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)
 {

Signature · 1 line

1.8.4.12.g2ea3df6

Aaron Lu

author

2013-09-24 09:54 UTC #3 lore raw

Re: [PATCH v3 2/4] ACPI / video: seperate backlight control and event interface

Quoted 551 lines · Aaron Lu

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 | 434 +++++++++++++++++++++++++++++----------------------
 include/acpi/video.h |   2 +
 2 files changed, 247 insertions(+), 189 deletions(-)

drivers/acpi/video.c +245 ~~-189~~

index aebcf63..3bd1eaa 100644
@@ -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;

include/acpi/video.h +0 -0

index 61109f2..0c665b5 100644

include/acpi/video.h struct acpi_device; · line 22

Quoted 4 lines

include/acpi/video.h +1 -0

@@ -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);

Oops, forgot to remove the declration and the stub below. But it doesn't affect the test of the patches, so I'll leave it for a while till next revision.

-Aaron

Quoted 10 lines

include/acpi/video.h +1 -0

 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)
 {

Rafael J. Wysocki

2013-10-07 11:57 UTC #4 lore raw

Re: [PATCH v3 2/4] ACPI / video: seperate backlight control and event interface

Quoted 1 line · Aaron Lu

On 09/24/2013 05:47 PM, Aaron Lu wrote:

[...]

Quoted 5 lines

Quoted 5 lines
include/acpi/video.h +0 -0
index 61109f2..0c665b5 100644
@@ -19,11 +19,13 @@ struct acpi_device;

Quoted 8 lines

include/acpi/video.h struct acpi_device; · line 22
Quoted 4 lines
include/acpi/video.h +1 -0
 #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);
Oops, forgot to remove the declration and the stub below. But it doesn't affect the test of the patches, so I'll leave it for a while till next revision.

Any chance to send the next revision?

Rafael

Aaron Lu

author

2013-09-24 09:47 UTC #5 lore raw

PATCH v3 3/4 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.

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

---
 drivers/acpi/internal.h     |  5 ++---
 drivers/acpi/video.c        | 10 +++++-----
 drivers/acpi/video_detect.c | 14 ++++++++++++--
 3 files changed, 19 insertions(+), 10 deletions(-)

drivers/acpi/internal.h +2 -3

index 20f4233..453ae8d 100644
@@ -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_ */

drivers/acpi/video.c +5 -5

index 3bd1eaa..343db59 100644
@@ -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;

drivers/acpi/video_detect.c +12 -2

index 940edbf..23d7d26 100644
@@ -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.

Signature · 1 line

1.8.4.12.g2ea3df6

Rafael J. Wysocki

2013-09-25 17:41 UTC #6 lore raw

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

Quoted 8 lines · Aaron Lu

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".

Quoted 4 lines

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.

I think the idea is to use the aggressive default for now and we can switch the default back to the current behavior before the merge window in case there are too many problems with it?

Rafael

Quoted 108 lines

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

---
 drivers/acpi/internal.h     |  5 ++---
 drivers/acpi/video.c        | 10 +++++-----
 drivers/acpi/video_detect.c | 14 ++++++++++++--
 3 files changed, 19 insertions(+), 10 deletions(-)

drivers/acpi/internal.h +2 -3

index 20f4233..453ae8d 100644
@@ -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_ */

drivers/acpi/video.c +5 -5

index 3bd1eaa..343db59 100644
@@ -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;

drivers/acpi/video_detect.c +12 -2

index 940edbf..23d7d26 100644
@@ -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.

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Aaron Lu

author

2013-09-26 05:15 UTC #7 lore raw

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

Quoted 9 lines · Rafael J. Wysocki

Quoted 8 lines · Aaron Lu
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".

Quoted 8 lines

Quoted 4 lines
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.
I think the idea is to use the aggressive default for now and we can switch the default back to the current behavior before the merge window in case there are too many problems with it?

Yes I think so.

Thanks,
Aaron

Aaron Lu

author

2013-09-24 09:47 UTC #8 lore raw

PATCH v3 4/4 thinkpad-acpi: fix handle locate for video and query of _BCL

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

---
 drivers/platform/x86/thinkpad_acpi.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

drivers/platform/x86/thinkpad_acpi.c +24 -7

index 03ca6c1..170f278 100644
@@ -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);

Signature · 1 line

1.8.4.12.g2ea3df6

Henrique de Moraes Holschuh

2013-09-25 19:58 UTC #9 lore raw

Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL

Quoted 2 lines · Aaron Lu

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

ACPI video is supposed to attach a virtual HID node (ACPI_VIDEO_HID) to ACPI video devices so as to keep the non-trivial video device detection logic in just one place instead of reinventing the wheel in every driver (which is always a recipe for disaster).

When did that break?

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Aaron Lu

author

2013-09-26 01:43 UTC #10 lore raw

Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL

Quoted 10 lines · Henrique de Moraes Holschuh

Quoted 2 lines · Aaron Lu
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
ACPI video is supposed to attach a virtual HID node (ACPI_VIDEO_HID) to ACPI video devices so as to keep the non-trivial video device detection logic in just one place instead of reinventing the wheel in every driver (which is always a recipe for disaster).
When did that break?

I checked the git log for the commit to use tpacpi_acpi_handle_locate to locate video controller's ACPI handle, it's:

commit 122f26726b5e16174bf8a707df14be1d93c49d62

Author: Henrique de Moraes Holschuh <[email protected]>
Date:   Mon Aug 9 23:48:18 2010 -0300

thinkpad-acpi: find ACPI video device by synthetic HID

So I checked out that commit and found that it shouldn't work either, since it has the same problem of the current code.

The ACPI video controller device is given an id of ACPI_VIDEO_HID, but it's only known to Linux ACPI, not ACPICA, so the function provided by ACPICA acpi_get_devices will not work in this case, as that function will really check the control method of _HID under the handle, which does not exist no matter if Linux ACPI has added an id to its device structure or not. OTOH, the function provided by Linux ACPI acpi_device_hid will see the added id. In a word, the add of the HID will not affect the ASL namespace layout of the device node(thus, no _HID control method will be added to the device node).

It seems that this problem has been found previously by:

commit ff413195e830541afeae469fc866ecd0319abd7e

Author: Alex Hung <[email protected]>
Date:   Tue Apr 24 16:40:52 2012 +0800

thinkpad-acpi: fix issuing duplicated key events for brightness up/down

The tp_features.bright_acpimode will not be set correctly for brightness control because ACPI_VIDEO_HID will not be located in ACPI. As a result, a duplicated key event will always be sent. acpi_video_backlight_support() is sufficient to detect standard ACPI brightness control.

Signed-off-by Alex Hung <[email protected]>

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

Thanks,
Aaron

Henrique de Moraes Holschuh

2013-09-27 15:16 UTC #11 lore raw

Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL

Quoted 6 lines · Aaron Lu

I checked the git log for the commit to use tpacpi_acpi_handle_locate to locate video controller's ACPI handle, it's:
commit 122f26726b5e16174bf8a707df14be1d93c49d62
Author: Henrique de Moraes Holschuh <[email protected]>
Date:   Mon Aug 9 23:48:18 2010 -0300

Yeah...

Quoted 12 lines

So I checked out that commit and found that it shouldn't work either, since it has the same problem of the current code.
The ACPI video controller device is given an id of ACPI_VIDEO_HID, but it's only known to Linux ACPI, not ACPICA, so the function provided by ACPICA acpi_get_devices will not work in this case, as that function will really check the control method of _HID under the handle, which does not exist no matter if Linux ACPI has added an id to its device structure or not. OTOH, the function provided by Linux ACPI acpi_device_hid will see the added id. In a word, the add of the HID will not affect the ASL namespace layout of the device node(thus, no _HID control method will be added to the device node).

Erk. It went broken for a long time, and the users didn't notice(!)...

Quoted 6 lines

commit ff413195e830541afeae469fc866ecd0319abd7e
Author: Alex Hung <[email protected]>
Date:   Tue Apr 24 16:40:52 2012 +0800
thinkpad-acpi: fix issuing duplicated key events for brightness up/down

Quoted 7 lines

The tp_features.bright_acpimode will not be set correctly for brightness control because ACPI_VIDEO_HID will not be located in ACPI. As a result, a duplicated key event will always be sent. acpi_video_backlight_support() is sufficient to detect standard ACPI brightness control.
Signed-off-by Alex Hung <[email protected]>
Signed-off-by Matthew Garrett <[email protected]>

Until that. And unfortunately I did not connect the dots at the time.

Thanks for explaining the issue properly.

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Henrique de Moraes Holschuh

Acked-by on v3 4/4

2013-09-27 15:20 UTC #12 lore raw

Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL

Quoted 13 lines · Aaron Lu

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.

Quoted 5 lines

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 <[email protected]>
Tested-by Igor Gnatenko <[email protected]>

Some testing on a *60 (T60,X60...) would also be best, I cannot test this on my T43.

Anyway, the code itself looks fine, so:

Acked-by Henrique de Moraes Holschuh <[email protected]>

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Yves-Alexis Perez

2013-09-27 15:33 UTC #13 lore raw

Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL

Quoted 5 lines · Henrique de Moraes Holschuh

Some testing on a *60 (T60,X60...) would also be best, I cannot test this on
my T43.
Anyway, the code itself looks fine, so:

I can test on T61, would that help?

Regards,

Signature · 1 line

Yves-Alexis

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Henrique de Moraes Holschuh

2013-09-27 18:05 UTC #14 lore raw

Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL

Quoted 8 lines · Yves-Alexis Perez

Quoted 5 lines · Henrique de Moraes Holschuh
Some testing on a *60 (T60,X60...) would also be best, I cannot test this on
my T43.
Anyway, the code itself looks fine, so:
I can test on T61, would that help?

I think so.

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Yves-Alexis Perez

2013-09-28 12:28 UTC #15 lore raw

Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL

Quoted 6 lines · Henrique de Moraes Holschuh

Quoted 5 lines · Yves-Alexis Perez
Quoted 4 lines · Henrique de Moraes Holschuh
Some testing on a *60 (T60,X60...) would also be best, I cannot test this on
my T43.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Anyway, the code itself looks fine, so:
I can test on T61, would that help?
I think so.

Ok, just tested on my T61 with Intel graphics.

I've checked that on Linux 3.12 (6cac446bd37d9381815fe4c2b0e7b1fd1085000c), brightness keys work fine like they do in 3.2.

Then I've applied the patchset. The brightness keys still work, I still have 16 levels.

Regards,

Signature · 1 line

Yves-Alexis

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Igor Gnatenko

2013-09-24 12:42 UTC #16 lore raw

Quoted 33 lines · Aaron Lu

v3:
1 Add a new patch 4/4 to fix some problems in thinkpad-acpi module; 2 Remove unnecessary function acpi_video_unregister introduced in
patch 2/3 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 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.
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

Quoted 9 lines

 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/acpi/video.h                 |   2 +
 include/linux/backlight.h            |   4 +
 7 files changed, 324 insertions(+), 205 deletions(-)

Excellent! I've tested this patch-set on ThinkPad X230 and backlight issue is exhausted.

Thanks.

Signature · 3 lines

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

Jörg Otte

2013-09-25 08:29 UTC #17 lore raw

Backlight can't be modified with this patch set - neither with function keys nor
with the gui. It is a step backward to v3.11-rc1

Video driver: i915
FUJITSU LIFEBOOK AH532/FJNBB1C, BIOS Version 1.09 05/22/2012

acpi_backlight=video works.

Jörg

2013/9/24 Igor Gnatenko <[email protected]>

Quoted 48 lines · Aaron Lu

Quoted 33 lines
v3:
1 Add a new patch 4/4 to fix some problems in thinkpad-acpi module; 2 Remove unnecessary function acpi_video_unregister introduced in
patch 2/3 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 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.
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
Quoted 2 lines
 drivers/acpi/internal.h              |   5 +-
 drivers/acpi/video.c                 | 442
++++++++++++++++++++---------------
Quoted 7 lines
 drivers/acpi/video_detect.c          |  14 +-
 drivers/platform/x86/thinkpad_acpi.c |  31 ++-
 drivers/video/backlight/backlight.c  |  31 +++
 include/acpi/video.h                 |   2 +
 include/linux/backlight.h            |   4 +
 7 files changed, 324 insertions(+), 205 deletions(-)
Excellent! I've tested this patch-set on ThinkPad X230 and backlight issue is exhausted.

Quoted 8 lines

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

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Aaron Lu

author

2013-09-25 09:16 UTC #18 lore raw

Quoted 3 lines · Jörg Otte

Backlight can't be modified with this patch set - neither with function keys nor
with the gui. It is a step backward to v3.11-rc1

Thanks for the test.

Please check /sys/class/backlight, is there only one link file intel_backlight? If so, please cd inside and try modify the brightness file using echo with some values in the range of 0 - max_brightness, does the backlight level change when you echo a new value? I guess it doesn't, or you wouldn't notice problem. If indeed so, perhaps file a bug at http://bugzilla.kernel.org, Drivers/Video(DRI-Intel)? I suppose Jani and Daniel can help fix your problem.

Thanks,
Aaron

Quoted 4 lines

Video driver: i915
FUJITSU LIFEBOOK AH532/FJNBB1C, BIOS Version 1.09 05/22/2012

Quoted 63 lines

acpi_backlight=video works.
Jörg
2013/9/24 Igor Gnatenko <[email protected]>
Quoted 56 lines · Aaron Lu
Quoted 33 lines
v3:
1 Add a new patch 4/4 to fix some problems in thinkpad-acpi module; 2 Remove unnecessary function acpi_video_unregister introduced in
patch 2/3 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 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.
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
Quoted 2 lines
 drivers/acpi/internal.h              |   5 +-
 drivers/acpi/video.c                 | 442
++++++++++++++++++++---------------
Quoted 7 lines
 drivers/acpi/video_detect.c          |  14 +-
 drivers/platform/x86/thinkpad_acpi.c |  31 ++-
 drivers/video/backlight/backlight.c  |  31 +++
 include/acpi/video.h                 |   2 +
 include/linux/backlight.h            |   4 +
 7 files changed, 324 insertions(+), 205 deletions(-)
Excellent! I've tested this patch-set on ThinkPad X230 and backlight issue is exhausted.
Thanks.
--
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.1-300.fc20.x86_64

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

Jani Nikula

2013-09-25 10:39 UTC #19 lore raw

Quoted 3 lines · Aaron Lu

Quoted 2 lines · Jörg Otte
Backlight can't be modified with this patch set - neither with function keys nor with the gui. It is a step backward to v3.11-rc1

So both hotkeys and GUI work in v3.11-rc1? And v3.12-rc2?

Quoted 4 lines

Thanks for the test.
Please check /sys/class/backlight, is there only one link file intel_backlight?

Indeed, are there others? fujitsu-laptop perhaps? If yes, try CONFIG_FUJITSU_LAPTOP=n or an appropriate module blacklist.

Just checking, you do have CONFIG_BACKLIGHT_CLASS_DEVICE=y? Embarrassingly there was a bug in i915 fixed just recently where the backlight device wasn't registered for CONFIG_BACKLIGHT_CLASS_DEVICE=m...

Quoted 6 lines

If so, please cd inside and try modify the brightness file using echo with some values in the range of 0 - max_brightness, does the backlight level change when you echo a new value? I guess it doesn't, or you wouldn't notice problem. If indeed so, perhaps file a bug at
http://bugzilla.kernel.org, Drivers/Video(DRI-Intel)? I suppose Jani and Daniel can help fix your problem.

Yup, please check the above, and report back.

BR,
Jani.

Quoted 71 lines

Thanks,
Aaron
Quoted 67 lines
Video driver: i915
FUJITSU LIFEBOOK AH532/FJNBB1C, BIOS Version 1.09 05/22/2012
acpi_backlight=video works.
Jörg
2013/9/24 Igor Gnatenko <[email protected]>
Quoted 56 lines · Aaron Lu
Quoted 33 lines
v3:
1 Add a new patch 4/4 to fix some problems in thinkpad-acpi module; 2 Remove unnecessary function acpi_video_unregister introduced in
patch 2/3 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 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.
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
Quoted 2 lines
 drivers/acpi/internal.h              |   5 +-
 drivers/acpi/video.c                 | 442
++++++++++++++++++++---------------
Quoted 7 lines
 drivers/acpi/video_detect.c          |  14 +-
 drivers/platform/x86/thinkpad_acpi.c |  31 ++-
 drivers/video/backlight/backlight.c  |  31 +++
 include/acpi/video.h                 |   2 +
 include/linux/backlight.h            |   4 +
 7 files changed, 324 insertions(+), 205 deletions(-)
Excellent! I've tested this patch-set on ThinkPad X230 and backlight issue is exhausted.
Thanks.
--
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.1-300.fc20.x86_64

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Jörg Otte

2013-09-25 15:51 UTC #20 lore raw

2013/9/25 Jani Nikula <[email protected]>:

Quoted 6 lines · Aaron Lu

Quoted 3 lines · Jörg Otte
Quoted 2 lines
Backlight can't be modified with this patch set - neither with function keys nor with the gui. It is a step backward to v3.11-rc1
So both hotkeys and GUI work in v3.11-rc1? And v3.12-rc2?

In v3.11-rc1 it didn't work.
Since a later rc (I don't remember exactly which) it did work. In v3.12-rc1/2 it does work.
In v3.12-rc2 + Aaron's patch set it again doesn't work.

Quoted 10 lines

Quoted 5 lines
Thanks for the test.
Please check /sys/class/backlight, is there only one link file intel_backlight?
Indeed, are there others? fujitsu-laptop perhaps? If yes, try CONFIG_FUJITSU_LAPTOP=n or an appropriate module blacklist.
Just checking, you do have CONFIG_BACKLIGHT_CLASS_DEVICE=y?

There is only one intel_backlight link.
Yes, I have CONFIG_BACKLIGHT_CLASS_DEVICE=y

Quoted 4 lines

Embarrassingly there was a bug in i915 fixed just recently where the backlight device wasn't registered for CONFIG_BACKLIGHT_CLASS_DEVICE=m...

Quoted 8 lines

Quoted 6 lines
If so, please cd inside and try modify the brightness file using echo with some values in the range of 0 - max_brightness, does the backlight level change when you echo a new value? I guess it doesn't, or you wouldn't notice problem. If indeed so, perhaps file a bug at
http://bugzilla.kernel.org, Drivers/Video(DRI-Intel)? I suppose Jani and Daniel can help fix your problem.
Yup, please check the above, and report back.

- echo 0..max_brightness > brightness does not work.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Video driver: i915
FUJITSU LIFEBOOK AH532/FJNBB1C, BIOS Version 1.09 05/22/2012
acpi_backlight=video works.

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

Jani Nikula

2013-09-25 16:17 UTC #21 lore raw

Quoted 38 lines · Jörg Otte

2013/9/25 Jani Nikula <[email protected]>:
Quoted 6 lines · Aaron Lu
Quoted 3 lines · Jörg Otte
Quoted 2 lines
Backlight can't be modified with this patch set - neither with function keys nor with the gui. It is a step backward to v3.11-rc1
So both hotkeys and GUI work in v3.11-rc1? And v3.12-rc2?
In v3.11-rc1 it didn't work.
Since a later rc (I don't remember exactly which) it did work. In v3.12-rc1/2 it does work.
In v3.12-rc2 + Aaron's patch set it again doesn't work.
Quoted 10 lines
Quoted 5 lines
Thanks for the test.
Please check /sys/class/backlight, is there only one link file intel_backlight?
Indeed, are there others? fujitsu-laptop perhaps? If yes, try CONFIG_FUJITSU_LAPTOP=n or an appropriate module blacklist.
Just checking, you do have CONFIG_BACKLIGHT_CLASS_DEVICE=y?
There is only one intel_backlight link.
Yes, I have CONFIG_BACKLIGHT_CLASS_DEVICE=y
Quoted 11 lines
Embarrassingly there was a bug in i915 fixed just recently where the backlight device wasn't registered for CONFIG_BACKLIGHT_CLASS_DEVICE=m...
Quoted 6 lines
If so, please cd inside and try modify the brightness file using echo with some values in the range of 0 - max_brightness, does the backlight level change when you echo a new value? I guess it doesn't, or you wouldn't notice problem. If indeed so, perhaps file a bug at
http://bugzilla.kernel.org, Drivers/Video(DRI-Intel)? I suppose Jani and Daniel can help fix your problem.

Quoted 3 lines

Quoted 1 line
Yup, please check the above, and report back.
- echo 0..max_brightness > brightness does not work.

Thanks for the info.

How about v3.12-rc2 without Aaron's patches? Does intel_backlight also not work there? How about the acpi_video0 (which I presume is present) sysfs interface?

BR,
Jani.

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Video driver: i915
FUJITSU LIFEBOOK AH532/FJNBB1C, BIOS Version 1.09 05/22/2012
acpi_backlight=video works.
Jörg

Jörg Otte

2013-09-26 07:49 UTC #22 lore raw

2013/9/25 Jani Nikula <[email protected]>:

Quoted 41 lines · Jörg Otte

Quoted 40 lines
2013/9/25 Jani Nikula <[email protected]>:
Quoted 6 lines · Aaron Lu
Quoted 3 lines · Jörg Otte
Quoted 2 lines
Backlight can't be modified with this patch set - neither with function keys nor with the gui. It is a step backward to v3.11-rc1
So both hotkeys and GUI work in v3.11-rc1? And v3.12-rc2?
In v3.11-rc1 it didn't work.
Since a later rc (I don't remember exactly which) it did work. In v3.12-rc1/2 it does work.
In v3.12-rc2 + Aaron's patch set it again doesn't work.
Quoted 10 lines
Quoted 5 lines
Thanks for the test.
Please check /sys/class/backlight, is there only one link file intel_backlight?
Indeed, are there others? fujitsu-laptop perhaps? If yes, try CONFIG_FUJITSU_LAPTOP=n or an appropriate module blacklist.
Just checking, you do have CONFIG_BACKLIGHT_CLASS_DEVICE=y?
There is only one intel_backlight link.
Yes, I have CONFIG_BACKLIGHT_CLASS_DEVICE=y
Quoted 12 lines
Embarrassingly there was a bug in i915 fixed just recently where the backlight device wasn't registered for CONFIG_BACKLIGHT_CLASS_DEVICE=m...
Quoted 6 lines
If so, please cd inside and try modify the brightness file using echo with some values in the range of 0 - max_brightness, does the backlight level change when you echo a new value? I guess it doesn't, or you wouldn't notice problem. If indeed so, perhaps file a bug at
http://bugzilla.kernel.org, Drivers/Video(DRI-Intel)? I suppose Jani and Daniel can help fix your problem.
Yup, please check the above, and report back.

Quoted 8 lines

Quoted 1 line
- echo 0..max_brightness > brightness does not work.
Thanks for the info.
How about v3.12-rc2 without Aaron's patches? Does intel_backlight also not work there? How about the acpi_video0 (which I presume is present) sysfs interface?

Without Aaron's patches intel_backlight also does not work. But acpi_video0 does (and takes precedence I think).

Quoted 4 lines

Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Video driver: i915
FUJITSU LIFEBOOK AH532/FJNBB1C, BIOS Version 1.09 05/22/2012
Jörg

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Rafael J. Wysocki

2013-09-27 00:15 UTC #23 lore raw

Quoted 46 lines · Jörg Otte

2013/9/25 Jani Nikula <[email protected]>:
Quoted 45 lines · Jörg Otte
Quoted 38 lines
2013/9/25 Jani Nikula <[email protected]>:
Quoted 6 lines · Aaron Lu
Quoted 3 lines · Jörg Otte
Quoted 2 lines
Backlight can't be modified with this patch set - neither with function keys nor with the gui. It is a step backward to v3.11-rc1
So both hotkeys and GUI work in v3.11-rc1? And v3.12-rc2?
In v3.11-rc1 it didn't work.
Since a later rc (I don't remember exactly which) it did work. In v3.12-rc1/2 it does work.
In v3.12-rc2 + Aaron's patch set it again doesn't work.
Quoted 10 lines
Quoted 5 lines
Thanks for the test.
Please check /sys/class/backlight, is there only one link file intel_backlight?
Indeed, are there others? fujitsu-laptop perhaps? If yes, try CONFIG_FUJITSU_LAPTOP=n or an appropriate module blacklist.
Just checking, you do have CONFIG_BACKLIGHT_CLASS_DEVICE=y?
There is only one intel_backlight link.
Yes, I have CONFIG_BACKLIGHT_CLASS_DEVICE=y
Quoted 11 lines
Embarrassingly there was a bug in i915 fixed just recently where the backlight device wasn't registered for CONFIG_BACKLIGHT_CLASS_DEVICE=m...
Quoted 6 lines
If so, please cd inside and try modify the brightness file using echo with some values in the range of 0 - max_brightness, does the backlight level change when you echo a new value? I guess it doesn't, or you wouldn't notice problem. If indeed so, perhaps file a bug at
http://bugzilla.kernel.org, Drivers/Video(DRI-Intel)? I suppose Jani and Daniel can help fix your problem.
Quoted 3 lines
Quoted 1 line
Yup, please check the above, and report back.
- echo 0..max_brightness > brightness does not work.
Thanks for the info.

Quoted 7 lines

Quoted 4 lines
How about v3.12-rc2 without Aaron's patches? Does intel_backlight also not work there? How about the acpi_video0 (which I presume is present) sysfs interface?
Without Aaron's patches intel_backlight also does not work. But acpi_video0 does (and takes precedence I think).

So can you please open a bug entry at bugzilla.kernel.org against i915?

The fact that ACPI video works for you doesn't mean that intel_backlight shouldn't, I suppose?

Rafael

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

Jörg Otte

2013-09-28 16:34 UTC #24 lore raw

2013/9/27 Rafael J. Wysocki <[email protected]>:

Quoted 55 lines · Jörg Otte

Quoted 46 lines
2013/9/25 Jani Nikula <[email protected]>:
Quoted 45 lines · Jörg Otte
Quoted 38 lines
2013/9/25 Jani Nikula <[email protected]>:
Quoted 6 lines · Aaron Lu
Quoted 3 lines · Jörg Otte
Quoted 2 lines
Backlight can't be modified with this patch set - neither with function keys nor with the gui. It is a step backward to v3.11-rc1
So both hotkeys and GUI work in v3.11-rc1? And v3.12-rc2?
In v3.11-rc1 it didn't work.
Since a later rc (I don't remember exactly which) it did work. In v3.12-rc1/2 it does work.
In v3.12-rc2 + Aaron's patch set it again doesn't work.
Quoted 10 lines
Quoted 5 lines
Thanks for the test.
Please check /sys/class/backlight, is there only one link file intel_backlight?
Indeed, are there others? fujitsu-laptop perhaps? If yes, try CONFIG_FUJITSU_LAPTOP=n or an appropriate module blacklist.
Just checking, you do have CONFIG_BACKLIGHT_CLASS_DEVICE=y?
There is only one intel_backlight link.
Yes, I have CONFIG_BACKLIGHT_CLASS_DEVICE=y
Quoted 11 lines
Embarrassingly there was a bug in i915 fixed just recently where the backlight device wasn't registered for CONFIG_BACKLIGHT_CLASS_DEVICE=m...
Quoted 6 lines
If so, please cd inside and try modify the brightness file using echo with some values in the range of 0 - max_brightness, does the backlight level change when you echo a new value? I guess it doesn't, or you wouldn't notice problem. If indeed so, perhaps file a bug at
http://bugzilla.kernel.org, Drivers/Video(DRI-Intel)? I suppose Jani and Daniel can help fix your problem.
Quoted 3 lines
Quoted 1 line
Yup, please check the above, and report back.
- echo 0..max_brightness > brightness does not work.
Thanks for the info.
Quoted 7 lines
Quoted 4 lines
How about v3.12-rc2 without Aaron's patches? Does intel_backlight also not work there? How about the acpi_video0 (which I presume is present) sysfs interface?
Without Aaron's patches intel_backlight also does not work. But acpi_video0 does (and takes precedence I think).

Quoted 7 lines

So can you please open a bug entry at bugzilla.kernel.org against i915?
The fact that ACPI video works for you doesn't mean that intel_backlight shouldn't, I suppose?
Rafael

done. Bug-Id:62281

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

Jörg Otte

2013-09-25 09:09 UTC #25 lore raw

2013/9/24 Aaron Lu <[email protected]>:

Quoted 42 lines

v3:
1 Add a new patch 4/4 to fix some problems in thinkpad-acpi module; 2 Remove unnecessary function acpi_video_unregister introduced in
patch 2/3 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 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.
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/acpi/video.h                 |   2 +
 include/linux/backlight.h            |   4 +
 7 files changed, 324 insertions(+), 205 deletions(-)

Quoted 3 lines

--
1.8.4.12.g2ea3df6

Backlight can't be modified with this patch set - neither with function keys nor
with the gui. It is a step backward to v3.11-rc1

Video driver: i915
FUJITSU LIFEBOOK AH532/FJNBB1C, BIOS Version 1.09 05/22/2012

acpi_backlight=video works.

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

Mika Westerberg

Tested-by on the v3 series

2013-09-27 10:57 UTC #26 lore raw