[PATCH 0/2] Rework ACPI video driver

[PATCH 0/2] Rework ACPI video driver

[Aaron Lu]

The ACPI video module provides two functionalities: backlight control
and backlight hotkey event delivery. It is possible the backlight
control interface is broken while the system still needs its event
delivery, so it's worth to seperate the two interfaces clearly.

This patchset has two patches, the first is to sepeate the two
interfaces in the ACPI video module and the second one solves some
Win8 backlight control problems by removing ACPI video's backlight
interface while still keeping its event delivery functionality. Due
to some systems whose firmware claims win8 compatible have problems
with i915's backlight control interface, a module param is introduced
to give user a chance to select if they want to remove ACPI video's
backlight control interface. The param is set to false by default.

Aaron Lu (2):
  ACPI / video: seperate backlight control and event interface
  ACPI / video / i915: Remove ACPI backlight if firmware expects Windows
    8

 drivers/acpi/internal.h         |   2 +
 drivers/acpi/video.c            | 473 +++++++++++++++++++++++-----------------
 drivers/acpi/video_detect.c     |  15 +-
 drivers/gpu/drm/i915/i915_dma.c |   2 +-
 drivers/gpu/drm/i915/i915_drv.c |   5 +
 drivers/gpu/drm/i915/i915_drv.h |   1 +
 include/acpi/video.h            |  11 +-
 include/linux/acpi.h            |   1 +
 8 files changed, 310 insertions(+), 200 deletions(-)

-- 
1.8.4.12.g2ea3df6

[PATCH 1/2] ACPI / video: seperate backlight control and event interface

[Aaron Lu]

The backlight control and event delivery functionality provided by ACPI
video module is mixed together and registered all during video device
enumeration time. As a result, the two functionality are also removed
together on module unload time or by the acpi_video_unregister function.
The two functionalities are actually independent and one may be useful
while the other one may be broken, so it is desirable to seperate the
two functionalities such that it is clear and easy to disable one
functionality without affecting the other one. This patch does the
seperation and as a result, a new video_bus_head list is introduced to
store all registered video bus structure and a new function
acpi_video_unregister_backlight is introduced to unregister backlight
interfaces for all video devices belonging to stored video buses.

Currently, there is no need to unregister ACPI video's event delivery
functionality alone so the function acpi_video_remove_notify_handler is
not introduced, it can be easily added when needed.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/acpi/video.c | 451 ++++++++++++++++++++++++++++++---------------------
 include/acpi/video.h |   2 +
 2 files changed, 264 insertions(+), 189 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index aebcf63..5ad5a71 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -89,6 +89,8 @@ static bool use_bios_initial_backlight = 1;
 module_param(use_bios_initial_backlight, bool, 0644);
 
 static int register_count;
+static struct mutex video_list_lock;
+static struct list_head video_bus_head;
 static int acpi_video_bus_add(struct acpi_device *device);
 static int acpi_video_bus_remove(struct acpi_device *device);
 static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
@@ -157,6 +159,7 @@ struct acpi_video_bus {
 	struct acpi_video_bus_flags flags;
 	struct list_head video_device_list;
 	struct mutex device_list_lock;	/* protects video_device_list */
+	struct list_head entry;
 	struct input_dev *input;
 	char phys[32];	/* for input device */
 	struct notifier_block pm_nb;
@@ -884,79 +887,6 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 
 	if (acpi_has_method(device->dev->handle, "_DDC"))
 		device->cap._DDC = 1;
-
-	if (acpi_video_backlight_support()) {
-		struct backlight_properties props;
-		struct pci_dev *pdev;
-		acpi_handle acpi_parent;
-		struct device *parent = NULL;
-		int result;
-		static int count;
-		char *name;
-
-		result = acpi_video_init_brightness(device);
-		if (result)
-			return;
-		name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
-		if (!name)
-			return;
-		count++;
-
-		acpi_get_parent(device->dev->handle, &acpi_parent);
-
-		pdev = acpi_get_pci_dev(acpi_parent);
-		if (pdev) {
-			parent = &pdev->dev;
-			pci_dev_put(pdev);
-		}
-
-		memset(&props, 0, sizeof(struct backlight_properties));
-		props.type = BACKLIGHT_FIRMWARE;
-		props.max_brightness = device->brightness->count - 3;
-		device->backlight = backlight_device_register(name,
-							      parent,
-							      device,
-							      &acpi_backlight_ops,
-							      &props);
-		kfree(name);
-		if (IS_ERR(device->backlight))
-			return;
-
-		/*
-		 * Save current brightness level in case we have to restore it
-		 * before acpi_video_device_lcd_set_level() is called next time.
-		 */
-		device->backlight->props.brightness =
-				acpi_video_get_brightness(device->backlight);
-
-		device->cooling_dev = thermal_cooling_device_register("LCD",
-					device->dev, &video_cooling_ops);
-		if (IS_ERR(device->cooling_dev)) {
-			/*
-			 * Set cooling_dev to NULL so we don't crash trying to
-			 * free it.
-			 * Also, why the hell we are returning early and
-			 * not attempt to register video output if cooling
-			 * device registration failed?
-			 * -- dtor
-			 */
-			device->cooling_dev = NULL;
-			return;
-		}
-
-		dev_info(&device->dev->dev, "registered as cooling_device%d\n",
-			 device->cooling_dev->id);
-		result = sysfs_create_link(&device->dev->dev.kobj,
-				&device->cooling_dev->device.kobj,
-				"thermal_cooling");
-		if (result)
-			printk(KERN_ERR PREFIX "Create sysfs link\n");
-		result = sysfs_create_link(&device->cooling_dev->device.kobj,
-				&device->dev->dev.kobj, "device");
-		if (result)
-			printk(KERN_ERR PREFIX "Create sysfs link\n");
-
-	}
 }
 
 /*
@@ -1143,13 +1073,6 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
 	acpi_video_device_bind(video, data);
 	acpi_video_device_find_cap(data);
 
-	status = acpi_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
-					     acpi_video_device_notify, data);
-	if (ACPI_FAILURE(status))
-		dev_err(&device->dev, "Error installing notify handler\n");
-	else
-		data->flags.notify = 1;
-
 	mutex_lock(&video->device_list_lock);
 	list_add_tail(&data->entry, &video->video_device_list);
 	mutex_unlock(&video->device_list_lock);
@@ -1454,64 +1377,6 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video,
 	return status;
 }
 
-static int acpi_video_bus_put_one_device(struct acpi_video_device *device)
-{
-	acpi_status status;
-
-	if (!device || !device->video)
-		return -ENOENT;
-
-	if (device->flags.notify) {
-		status = acpi_remove_notify_handler(device->dev->handle,
-				ACPI_DEVICE_NOTIFY, acpi_video_device_notify);
-		if (ACPI_FAILURE(status))
-			dev_err(&device->dev->dev,
-					"Can't remove video notify handler\n");
-	}
-
-	if (device->backlight) {
-		backlight_device_unregister(device->backlight);
-		device->backlight = NULL;
-	}
-	if (device->cooling_dev) {
-		sysfs_remove_link(&device->dev->dev.kobj,
-				  "thermal_cooling");
-		sysfs_remove_link(&device->cooling_dev->device.kobj,
-				  "device");
-		thermal_cooling_device_unregister(device->cooling_dev);
-		device->cooling_dev = NULL;
-	}
-
-	return 0;
-}
-
-static int acpi_video_bus_put_devices(struct acpi_video_bus *video)
-{
-	int status;
-	struct acpi_video_device *dev, *next;
-
-	mutex_lock(&video->device_list_lock);
-
-	list_for_each_entry_safe(dev, next, &video->video_device_list, entry) {
-
-		status = acpi_video_bus_put_one_device(dev);
-		if (ACPI_FAILURE(status))
-			printk(KERN_WARNING PREFIX
-			       "hhuuhhuu bug in acpi video driver.\n");
-
-		if (dev->brightness) {
-			kfree(dev->brightness->levels);
-			kfree(dev->brightness);
-		}
-		list_del(&dev->entry);
-		kfree(dev);
-	}
-
-	mutex_unlock(&video->device_list_lock);
-
-	return 0;
-}
-
 /* acpi_video interface */
 
 /*
@@ -1536,7 +1401,7 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
 	struct input_dev *input;
 	int keycode = 0;
 
-	if (!video)
+	if (!video || !video->input)
 		return;
 
 	input = video->input;
@@ -1691,12 +1556,253 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
 	return AE_OK;
 }
 
+static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
+{
+	if (acpi_video_backlight_support()) {
+		struct backlight_properties props;
+		struct pci_dev *pdev;
+		acpi_handle acpi_parent;
+		struct device *parent = NULL;
+		int result;
+		static int count;
+		char *name;
+
+		result = acpi_video_init_brightness(device);
+		if (result)
+			return;
+		name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
+		if (!name)
+			return;
+		count++;
+
+		acpi_get_parent(device->dev->handle, &acpi_parent);
+
+		pdev = acpi_get_pci_dev(acpi_parent);
+		if (pdev) {
+			parent = &pdev->dev;
+			pci_dev_put(pdev);
+		}
+
+		memset(&props, 0, sizeof(struct backlight_properties));
+		props.type = BACKLIGHT_FIRMWARE;
+		props.max_brightness = device->brightness->count - 3;
+		device->backlight = backlight_device_register(name,
+							      parent,
+							      device,
+							      &acpi_backlight_ops,
+							      &props);
+		kfree(name);
+		if (IS_ERR(device->backlight))
+			return;
+
+		/*
+		 * Save current brightness level in case we have to restore it
+		 * before acpi_video_device_lcd_set_level() is called next time.
+		 */
+		device->backlight->props.brightness =
+				acpi_video_get_brightness(device->backlight);
+
+		device->cooling_dev = thermal_cooling_device_register("LCD",
+					device->dev, &video_cooling_ops);
+		if (IS_ERR(device->cooling_dev)) {
+			/*
+			 * Set cooling_dev to NULL so we don't crash trying to
+			 * free it.
+			 * Also, why the hell we are returning early and
+			 * not attempt to register video output if cooling
+			 * device registration failed?
+			 * -- dtor
+			 */
+			device->cooling_dev = NULL;
+			return;
+		}
+
+		dev_info(&device->dev->dev, "registered as cooling_device%d\n",
+			 device->cooling_dev->id);
+		result = sysfs_create_link(&device->dev->dev.kobj,
+				&device->cooling_dev->device.kobj,
+				"thermal_cooling");
+		if (result)
+			printk(KERN_ERR PREFIX "Create sysfs link\n");
+		result = sysfs_create_link(&device->cooling_dev->device.kobj,
+				&device->dev->dev.kobj, "device");
+		if (result)
+			printk(KERN_ERR PREFIX "Create sysfs link\n");
+	}
+}
+
+static int acpi_video_bus_register_backlight(struct acpi_video_bus *video)
+{
+	struct acpi_video_device *dev;
+
+	mutex_lock(&video->device_list_lock);
+	list_for_each_entry(dev, &video->video_device_list, entry)
+		acpi_video_dev_register_backlight(dev);
+	mutex_unlock(&video->device_list_lock);
+
+	video->pm_nb.notifier_call = acpi_video_resume;
+	video->pm_nb.priority = 0;
+	return register_pm_notifier(&video->pm_nb);
+}
+
+static void acpi_video_dev_unregister_backlight(struct acpi_video_device *device)
+{
+	if (device->backlight) {
+		backlight_device_unregister(device->backlight);
+		device->backlight = NULL;
+	}
+	if (device->brightness) {
+		kfree(device->brightness->levels);
+		kfree(device->brightness);
+		device->brightness = NULL;
+	}
+	if (device->cooling_dev) {
+		sysfs_remove_link(&device->dev->dev.kobj, "thermal_cooling");
+		sysfs_remove_link(&device->cooling_dev->device.kobj, "device");
+		thermal_cooling_device_unregister(device->cooling_dev);
+		device->cooling_dev = NULL;
+	}
+}
+
+static int acpi_video_bus_unregister_backlight(struct acpi_video_bus *video)
+{
+	struct acpi_video_device *dev;
+	int error = unregister_pm_notifier(&video->pm_nb);
+
+	mutex_lock(&video->device_list_lock);
+	list_for_each_entry(dev, &video->video_device_list, entry)
+		acpi_video_dev_unregister_backlight(dev);
+	mutex_unlock(&video->device_list_lock);
+
+	return error;
+}
+
+int acpi_video_unregister_backlight(void)
+{
+	struct acpi_video_bus *video;
+	int error = 0;
+
+	mutex_lock(&video_list_lock);
+	list_for_each_entry(video, &video_bus_head, entry) {
+		error = acpi_video_bus_unregister_backlight(video);
+		if (error)
+			break;
+	}
+	mutex_unlock(&video_list_lock);
+
+	return error;
+}
+EXPORT_SYMBOL(acpi_video_unregister_backlight);
+
+static void acpi_video_dev_add_notify_handler(struct acpi_video_device *device)
+{
+	acpi_status status;
+	struct acpi_device *adev = device->dev;
+
+	status = acpi_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY,
+					     acpi_video_device_notify, device);
+	if (ACPI_FAILURE(status))
+		dev_err(&adev->dev, "Error installing notify handler\n");
+	else
+		device->flags.notify = 1;
+}
+
+static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video)
+{
+	struct input_dev *input;
+	struct acpi_video_device *dev;
+	int error;
+
+	video->input = input = input_allocate_device();
+	if (!input) {
+		error = -ENOMEM;
+		goto out;
+	}
+
+	error = acpi_video_bus_start_devices(video);
+	if (error)
+		goto err_free_input;
+
+	snprintf(video->phys, sizeof(video->phys),
+			"%s/video/input0", acpi_device_hid(video->device));
+
+	input->name = acpi_device_name(video->device);
+	input->phys = video->phys;
+	input->id.bustype = BUS_HOST;
+	input->id.product = 0x06;
+	input->dev.parent = &video->device->dev;
+	input->evbit[0] = BIT(EV_KEY);
+	set_bit(KEY_SWITCHVIDEOMODE, input->keybit);
+	set_bit(KEY_VIDEO_NEXT, input->keybit);
+	set_bit(KEY_VIDEO_PREV, input->keybit);
+	set_bit(KEY_BRIGHTNESS_CYCLE, input->keybit);
+	set_bit(KEY_BRIGHTNESSUP, input->keybit);
+	set_bit(KEY_BRIGHTNESSDOWN, input->keybit);
+	set_bit(KEY_BRIGHTNESS_ZERO, input->keybit);
+	set_bit(KEY_DISPLAY_OFF, input->keybit);
+
+	error = input_register_device(input);
+	if (error)
+		goto err_stop_dev;
+
+	mutex_lock(&video->device_list_lock);
+	list_for_each_entry(dev, &video->video_device_list, entry)
+		acpi_video_dev_add_notify_handler(dev);
+	mutex_unlock(&video->device_list_lock);
+
+	return 0;
+
+err_stop_dev:
+	acpi_video_bus_stop_devices(video);
+err_free_input:
+	input_free_device(input);
+	video->input = NULL;
+out:
+	return error;
+}
+
+static void acpi_video_dev_remove_notify_handler(struct acpi_video_device *dev)
+{
+	if (dev->flags.notify) {
+		acpi_remove_notify_handler(dev->dev->handle, ACPI_DEVICE_NOTIFY,
+					   acpi_video_device_notify);
+		dev->flags.notify = 0;
+	}
+}
+
+static void acpi_video_bus_remove_notify_handler(struct acpi_video_bus *video)
+{
+	struct acpi_video_device *dev;
+
+	mutex_lock(&video->device_list_lock);
+	list_for_each_entry(dev, &video->video_device_list, entry)
+		acpi_video_dev_remove_notify_handler(dev);
+	mutex_unlock(&video->device_list_lock);
+
+	acpi_video_bus_stop_devices(video);
+	input_unregister_device(video->input);
+	video->input = NULL;
+}
+
+static int acpi_video_bus_put_devices(struct acpi_video_bus *video)
+{
+	struct acpi_video_device *dev, *next;
+
+	mutex_lock(&video->device_list_lock);
+	list_for_each_entry_safe(dev, next, &video->video_device_list, entry) {
+		list_del(&dev->entry);
+		kfree(dev);
+	}
+	mutex_unlock(&video->device_list_lock);
+
+	return 0;
+}
+
 static int instance;
 
 static int acpi_video_bus_add(struct acpi_device *device)
 {
 	struct acpi_video_bus *video;
-	struct input_dev *input;
 	int error;
 	acpi_status status;
 
@@ -1748,62 +1854,24 @@ static int acpi_video_bus_add(struct acpi_device *device)
 	if (error)
 		goto err_put_video;
 
-	video->input = input = input_allocate_device();
-	if (!input) {
-		error = -ENOMEM;
-		goto err_put_video;
-	}
-
-	error = acpi_video_bus_start_devices(video);
-	if (error)
-		goto err_free_input_dev;
-
-	snprintf(video->phys, sizeof(video->phys),
-		"%s/video/input0", acpi_device_hid(video->device));
-
-	input->name = acpi_device_name(video->device);
-	input->phys = video->phys;
-	input->id.bustype = BUS_HOST;
-	input->id.product = 0x06;
-	input->dev.parent = &device->dev;
-	input->evbit[0] = BIT(EV_KEY);
-	set_bit(KEY_SWITCHVIDEOMODE, input->keybit);
-	set_bit(KEY_VIDEO_NEXT, input->keybit);
-	set_bit(KEY_VIDEO_PREV, input->keybit);
-	set_bit(KEY_BRIGHTNESS_CYCLE, input->keybit);
-	set_bit(KEY_BRIGHTNESSUP, input->keybit);
-	set_bit(KEY_BRIGHTNESSDOWN, input->keybit);
-	set_bit(KEY_BRIGHTNESS_ZERO, input->keybit);
-	set_bit(KEY_DISPLAY_OFF, input->keybit);
-
 	printk(KERN_INFO PREFIX "%s [%s] (multi-head: %s  rom: %s  post: %s)\n",
 	       ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device),
 	       video->flags.multihead ? "yes" : "no",
 	       video->flags.rom ? "yes" : "no",
 	       video->flags.post ? "yes" : "no");
+	mutex_lock(&video_list_lock);
+	list_add_tail(&video->entry, &video_bus_head);
+	mutex_unlock(&video_list_lock);
 
-	video->pm_nb.notifier_call = acpi_video_resume;
-	video->pm_nb.priority = 0;
-	error = register_pm_notifier(&video->pm_nb);
-	if (error)
-		goto err_stop_video;
-
-	error = input_register_device(input);
-	if (error)
-		goto err_unregister_pm_notifier;
+	acpi_video_bus_register_backlight(video);
+	acpi_video_bus_add_notify_handler(video);
 
 	return 0;
 
- err_unregister_pm_notifier:
-	unregister_pm_notifier(&video->pm_nb);
- err_stop_video:
-	acpi_video_bus_stop_devices(video);
- err_free_input_dev:
-	input_free_device(input);
- err_put_video:
+err_put_video:
 	acpi_video_bus_put_devices(video);
 	kfree(video->attached_array);
- err_free_video:
+err_free_video:
 	kfree(video);
 	device->driver_data = NULL;
 
@@ -1820,12 +1888,14 @@ static int acpi_video_bus_remove(struct acpi_device *device)
 
 	video = acpi_driver_data(device);
 
-	unregister_pm_notifier(&video->pm_nb);
-
-	acpi_video_bus_stop_devices(video);
+	acpi_video_bus_remove_notify_handler(video);
+	acpi_video_bus_unregister_backlight(video);
 	acpi_video_bus_put_devices(video);
 
-	input_unregister_device(video->input);
+	mutex_lock(&video_list_lock);
+	list_del(&video->entry);
+	mutex_unlock(&video_list_lock);
+
 	kfree(video->attached_array);
 	kfree(video);
 
@@ -1874,6 +1944,9 @@ int acpi_video_register(void)
 		return 0;
 	}
 
+	mutex_init(&video_list_lock);
+	INIT_LIST_HEAD(&video_bus_head);
+
 	result = acpi_bus_register_driver(&acpi_video_bus);
 	if (result < 0)
 		return -ENODEV;
diff --git a/include/acpi/video.h b/include/acpi/video.h
index 61109f2..0c665b5 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -19,11 +19,13 @@ struct acpi_device;
 #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
 extern int acpi_video_register(void);
 extern void acpi_video_unregister(void);
+extern int acpi_video_unregister_backlight(void);
 extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
 #else
 static inline int acpi_video_register(void) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
+static inline int acpi_video_unregister_backlight(void) { return; }
 static inline int acpi_video_get_edid(struct acpi_device *device, int type,
 				      int device_id, void **edid)
 {
-- 
1.8.4.12.g2ea3df6

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

[Igor Gnatenko]

On Mon, 2013-09-09 at 16:40 +0800, Aaron Lu wrote:
> The backlight control and event delivery functionality provided by ACPI
> video module is mixed together and registered all during video device
> enumeration time. As a result, the two functionality are also removed
> together on module unload time or by the acpi_video_unregister function.
> The two functionalities are actually independent and one may be useful
> while the other one may be broken, so it is desirable to seperate the
> two functionalities such that it is clear and easy to disable one
> functionality without affecting the other one. This patch does the
> seperation and as a result, a new video_bus_head list is introduced to
> store all registered video bus structure and a new function
> acpi_video_unregister_backlight is introduced to unregister backlight
> interfaces for all video devices belonging to stored video buses.
> 
> Currently, there is no need to unregister ACPI video's event delivery
> functionality alone so the function acpi_video_remove_notify_handler is
> not introduced, it can be easily added when needed.
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
> ---
>  drivers/acpi/video.c | 451 ++++++++++++++++++++++++++++++---------------------
>  include/acpi/video.h |   2 +
>  2 files changed, 264 insertions(+), 189 deletions(-)

-- 
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.0-3.fc20.x86_64

-- 
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.0-1.fc20.x86_64

[PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

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

There's a problem with that approach, however, because simply
avoiding to register the ACPI backlight interface if the firmware
calls _OSI for Windows 8 may not work in the following situations:
 (1) The ACPI backlight interface actually works on the given system
     and the i915 driver is not loaded (e.g. another graphics driver
     is used).
 (2) The ACPI backlight interface doesn't work on the given system,
     but there is a vendor platform driver that will register its
     own, equally broken, backlight interface if not prevented from
     doing so by the ACPI subsystem.
Therefore we need to allow the ACPI backlight interface to be
registered until the i915 driver is loaded which then will unregister
it if the firmware has called _OSI for Windows 8 (or will register
the ACPI video driver without backlight support if not already
present).

For this reason, introduce an alternative function for registering
ACPI video, __acpi_video_register(bool), that if ture is passed,
will check whether or not the ACPI video driver has already been
registered and whether or not the backlight Windows 8 quirk has to
be applied. If the quirk has to be applied, it will block the ACPI
backlight support and either unregister the backlight interface if
the ACPI video driver has already been registered, or register the
ACPI video driver without the backlight interface otherwise.  Make
the i915 driver use __acpi_video_register() instead of
acpi_video_register() in i915_driver_load(), and the param passed
there is controlled by the i915 module level parameter
i915_take_over_backlight, which is set to false by default.

This change is evolved from earlier patches of Matthew Garrett,
Chun-Yi Lee and Seth Forshee and is heavily based on two patches
from Rafael:
https://lkml.org/lkml/2013/7/17/720
https://lkml.org/lkml/2013/7/24/806

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/acpi/internal.h         |  2 ++
 drivers/acpi/video.c            | 24 ++++++++++++++++--------
 drivers/acpi/video_detect.c     | 15 ++++++++++++++-
 drivers/gpu/drm/i915/i915_dma.c |  2 +-
 drivers/gpu/drm/i915/i915_drv.c |  5 +++++
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 include/acpi/video.h            |  9 +++++++--
 include/linux/acpi.h            |  1 +
 8 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 20f4233..a53832e 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev,
   -------------------------------------------------------------------------- */
 #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
 bool acpi_video_backlight_quirks(void);
+bool acpi_video_verify_backlight_support(void);
 #else
 static inline bool acpi_video_backlight_quirks(void) { return false; }
+static inline bool acpi_video_verify_backlight_support(void) { return false; }
 #endif
 
 #endif /* _ACPI_INTERNAL_H_ */
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 5ad5a71..cc709a7 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
 	unsigned long long level_current, level_next;
 	int result = -EINVAL;
 
-	/* no warning message if acpi_backlight=vendor is used */
-	if (!acpi_video_backlight_support())
+	/* no warning message if acpi_backlight=vendor or a quirk is used */
+	if (!acpi_video_verify_backlight_support())
 		return 0;
 
 	if (!device->brightness)
@@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
 
 static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
 {
-	if (acpi_video_backlight_support()) {
+	if (acpi_video_verify_backlight_support()) {
 		struct backlight_properties props;
 		struct pci_dev *pdev;
 		acpi_handle acpi_parent;
@@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void)
 	return opregion;
 }
 
-int acpi_video_register(void)
+int __acpi_video_register(bool backlight_quirks)
 {
-	int result = 0;
+	bool no_backlight;
+	int result;
+
+	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
+
 	if (register_count) {
 		/*
-		 * if the function of acpi_video_register is already called,
-		 * don't register the acpi_vide_bus again and return no error.
+		 * If acpi_video_register() has been called already, don't try
+		 * to register acpi_video_bus, but unregister backlight devices
+		 * if no backlight support is requested.
 		 */
+		if (no_backlight)
+			acpi_video_unregister_backlight();
+
 		return 0;
 	}
 
@@ -1959,7 +1967,7 @@ int acpi_video_register(void)
 
 	return 0;
 }
-EXPORT_SYMBOL(acpi_video_register);
+EXPORT_SYMBOL(__acpi_video_register);
 
 void acpi_video_unregister(void)
 {
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index 940edbf..c7e43ec 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -235,7 +235,12 @@ static void acpi_video_caps_check(void)
 
 bool acpi_video_backlight_quirks(void)
 {
-	return acpi_gbl_osi_data >= ACPI_OSI_WIN_8;
+	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
+		acpi_video_caps_check();
+		acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT;
+		return true;
+	}
+	return false;
 }
 EXPORT_SYMBOL(acpi_video_backlight_quirks);
 
@@ -283,6 +288,14 @@ int acpi_video_backlight_support(void)
 }
 EXPORT_SYMBOL(acpi_video_backlight_support);
 
+/* For the ACPI video driver use only. */
+bool acpi_video_verify_backlight_support(void)
+{
+	return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ?
+		false : acpi_video_backlight_support();
+}
+EXPORT_SYMBOL(acpi_video_verify_backlight_support);
+
 /*
  * Use acpi_backlight=vendor/video to force that backlight switching
  * is processed by vendor specific acpi drivers or video.ko driver.
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index f466980..75fba17 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	if (INTEL_INFO(dev)->num_pipes) {
 		/* Must be done after probing outputs */
 		intel_opregion_init(dev);
-		acpi_video_register();
+		__acpi_video_register(i915_take_over_backlight);
 	}
 
 	if (IS_GEN5(dev))
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 45b3c03..b014398 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -132,6 +132,11 @@ int i915_enable_ips __read_mostly = 1;
 module_param_named(enable_ips, i915_enable_ips, int, 0600);
 MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
 
+bool i915_take_over_backlight = false;
+module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644);
+MODULE_PARM_DESC(take_over_backlight,
+		"Prevent ACPI backlight from being used (default: false)");
+
 static struct drm_driver driver;
 extern int intel_agp_enabled;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1929bff..b43c430 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly;
 extern unsigned int i915_preliminary_hw_support __read_mostly;
 extern int i915_disable_power_well __read_mostly;
 extern int i915_enable_ips __read_mostly;
+extern bool i915_take_over_backlight __read_mostly;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
 extern int i915_resume(struct drm_device *dev);
diff --git a/include/acpi/video.h b/include/acpi/video.h
index 0c665b5..dc5b8ad 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -17,13 +17,13 @@ struct acpi_device;
 #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
 
 #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
-extern int acpi_video_register(void);
+extern int __acpi_video_register(bool backlight_quirks);
 extern void acpi_video_unregister(void);
 extern int acpi_video_unregister_backlight(void);
 extern int acpi_video_get_edid(struct acpi_device *device, int type,
 			       int device_id, void **edid);
 #else
-static inline int acpi_video_register(void) { return 0; }
+static inline int __acpi_video_register(bool backlight_quirks) { return 0; }
 static inline void acpi_video_unregister(void) { return; }
 static inline int acpi_video_unregister_backlight(void) { return; }
 static inline int acpi_video_get_edid(struct acpi_device *device, int type,
@@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
 }
 #endif
 
+static inline int acpi_video_register(void)
+{
+	return __acpi_video_register(false);
+}
+
 #endif
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a5db4ae..a0dcce5 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid);
 #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
 #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
+#define ACPI_VIDEO_SKIP_BACKLIGHT			0x1000
 
 #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
 
-- 
1.8.4.12.g2ea3df6

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Daniel Vetter]

Hi Aaaron,

Have we grown any clue meanwhile about which Intel boxes need this and for
which we still need to keep the acpi backlight around? I've grown _very_
reluctant to just adding tons of quirks to our driver for the backlight.
Almost all the quirks we have added recently (or that have been proposed
to be added) are for the backlight. Imo that indicates we get something
fundamentally wrong ...

Cheers, Daniel

On Mon, Sep 09, 2013 at 04:42:20PM +0800, Aaron Lu wrote:
> According to Matthew Garrett, "Windows 8 leaves backlight control up
> to individual graphics drivers rather than making ACPI calls itself.
> There's plenty of evidence to suggest that the Intel driver for
> Windows [8] doesn't use the ACPI interface, including the fact that
> it's broken on a bunch of machines when the OS claims to support
> Windows 8.  The simplest thing to do appears to be to disable the
> ACPI backlight interface on these systems".
> 
> There's a problem with that approach, however, because simply
> avoiding to register the ACPI backlight interface if the firmware
> calls _OSI for Windows 8 may not work in the following situations:
>  (1) The ACPI backlight interface actually works on the given system
>      and the i915 driver is not loaded (e.g. another graphics driver
>      is used).
>  (2) The ACPI backlight interface doesn't work on the given system,
>      but there is a vendor platform driver that will register its
>      own, equally broken, backlight interface if not prevented from
>      doing so by the ACPI subsystem.
> Therefore we need to allow the ACPI backlight interface to be
> registered until the i915 driver is loaded which then will unregister
> it if the firmware has called _OSI for Windows 8 (or will register
> the ACPI video driver without backlight support if not already
> present).
> 
> For this reason, introduce an alternative function for registering
> ACPI video, __acpi_video_register(bool), that if ture is passed,
> will check whether or not the ACPI video driver has already been
> registered and whether or not the backlight Windows 8 quirk has to
> be applied. If the quirk has to be applied, it will block the ACPI
> backlight support and either unregister the backlight interface if
> the ACPI video driver has already been registered, or register the
> ACPI video driver without the backlight interface otherwise.  Make
> the i915 driver use __acpi_video_register() instead of
> acpi_video_register() in i915_driver_load(), and the param passed
> there is controlled by the i915 module level parameter
> i915_take_over_backlight, which is set to false by default.
> 
> This change is evolved from earlier patches of Matthew Garrett,
> Chun-Yi Lee and Seth Forshee and is heavily based on two patches
> from Rafael:
> https://lkml.org/lkml/2013/7/17/720
> https://lkml.org/lkml/2013/7/24/806
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/acpi/internal.h         |  2 ++
>  drivers/acpi/video.c            | 24 ++++++++++++++++--------
>  drivers/acpi/video_detect.c     | 15 ++++++++++++++-
>  drivers/gpu/drm/i915/i915_dma.c |  2 +-
>  drivers/gpu/drm/i915/i915_drv.c |  5 +++++
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  include/acpi/video.h            |  9 +++++++--
>  include/linux/acpi.h            |  1 +
>  8 files changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 20f4233..a53832e 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev,
>    -------------------------------------------------------------------------- */
>  #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
>  bool acpi_video_backlight_quirks(void);
> +bool acpi_video_verify_backlight_support(void);
>  #else
>  static inline bool acpi_video_backlight_quirks(void) { return false; }
> +static inline bool acpi_video_verify_backlight_support(void) { return false; }
>  #endif
>  
>  #endif /* _ACPI_INTERNAL_H_ */
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 5ad5a71..cc709a7 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
>  	unsigned long long level_current, level_next;
>  	int result = -EINVAL;
>  
> -	/* no warning message if acpi_backlight=vendor is used */
> -	if (!acpi_video_backlight_support())
> +	/* no warning message if acpi_backlight=vendor or a quirk is used */
> +	if (!acpi_video_verify_backlight_support())
>  		return 0;
>  
>  	if (!device->brightness)
> @@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
>  
>  static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
>  {
> -	if (acpi_video_backlight_support()) {
> +	if (acpi_video_verify_backlight_support()) {
>  		struct backlight_properties props;
>  		struct pci_dev *pdev;
>  		acpi_handle acpi_parent;
> @@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void)
>  	return opregion;
>  }
>  
> -int acpi_video_register(void)
> +int __acpi_video_register(bool backlight_quirks)
>  {
> -	int result = 0;
> +	bool no_backlight;
> +	int result;
> +
> +	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
> +
>  	if (register_count) {
>  		/*
> -		 * if the function of acpi_video_register is already called,
> -		 * don't register the acpi_vide_bus again and return no error.
> +		 * If acpi_video_register() has been called already, don't try
> +		 * to register acpi_video_bus, but unregister backlight devices
> +		 * if no backlight support is requested.
>  		 */
> +		if (no_backlight)
> +			acpi_video_unregister_backlight();
> +
>  		return 0;
>  	}
>  
> @@ -1959,7 +1967,7 @@ int acpi_video_register(void)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL(acpi_video_register);
> +EXPORT_SYMBOL(__acpi_video_register);
>  
>  void acpi_video_unregister(void)
>  {
> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> index 940edbf..c7e43ec 100644
> --- a/drivers/acpi/video_detect.c
> +++ b/drivers/acpi/video_detect.c
> @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void)
>  
>  bool acpi_video_backlight_quirks(void)
>  {
> -	return acpi_gbl_osi_data >= ACPI_OSI_WIN_8;
> +	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
> +		acpi_video_caps_check();
> +		acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT;
> +		return true;
> +	}
> +	return false;
>  }
>  EXPORT_SYMBOL(acpi_video_backlight_quirks);
>  
> @@ -283,6 +288,14 @@ int acpi_video_backlight_support(void)
>  }
>  EXPORT_SYMBOL(acpi_video_backlight_support);
>  
> +/* For the ACPI video driver use only. */
> +bool acpi_video_verify_backlight_support(void)
> +{
> +	return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ?
> +		false : acpi_video_backlight_support();
> +}
> +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
> +
>  /*
>   * Use acpi_backlight=vendor/video to force that backlight switching
>   * is processed by vendor specific acpi drivers or video.ko driver.
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index f466980..75fba17 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	if (INTEL_INFO(dev)->num_pipes) {
>  		/* Must be done after probing outputs */
>  		intel_opregion_init(dev);
> -		acpi_video_register();
> +		__acpi_video_register(i915_take_over_backlight);
>  	}
>  
>  	if (IS_GEN5(dev))
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 45b3c03..b014398 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -132,6 +132,11 @@ int i915_enable_ips __read_mostly = 1;
>  module_param_named(enable_ips, i915_enable_ips, int, 0600);
>  MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
>  
> +bool i915_take_over_backlight = false;
> +module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644);
> +MODULE_PARM_DESC(take_over_backlight,
> +		"Prevent ACPI backlight from being used (default: false)");
> +
>  static struct drm_driver driver;
>  extern int intel_agp_enabled;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1929bff..b43c430 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly;
>  extern unsigned int i915_preliminary_hw_support __read_mostly;
>  extern int i915_disable_power_well __read_mostly;
>  extern int i915_enable_ips __read_mostly;
> +extern bool i915_take_over_backlight __read_mostly;
>  
>  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
>  extern int i915_resume(struct drm_device *dev);
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> index 0c665b5..dc5b8ad 100644
> --- a/include/acpi/video.h
> +++ b/include/acpi/video.h
> @@ -17,13 +17,13 @@ struct acpi_device;
>  #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
>  
>  #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
> -extern int acpi_video_register(void);
> +extern int __acpi_video_register(bool backlight_quirks);
>  extern void acpi_video_unregister(void);
>  extern int acpi_video_unregister_backlight(void);
>  extern int acpi_video_get_edid(struct acpi_device *device, int type,
>  			       int device_id, void **edid);
>  #else
> -static inline int acpi_video_register(void) { return 0; }
> +static inline int __acpi_video_register(bool backlight_quirks) { return 0; }
>  static inline void acpi_video_unregister(void) { return; }
>  static inline int acpi_video_unregister_backlight(void) { return; }
>  static inline int acpi_video_get_edid(struct acpi_device *device, int type,
> @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
>  }
>  #endif
>  
> +static inline int acpi_video_register(void)
> +{
> +	return __acpi_video_register(false);
> +}
> +
>  #endif
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index a5db4ae..a0dcce5 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid);
>  #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
>  #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
>  #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
> +#define ACPI_VIDEO_SKIP_BACKLIGHT			0x1000
>  
>  #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
>  
> -- 
> 1.8.4.12.g2ea3df6
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Rafael J. Wysocki]

Hi,

On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
> Hi Aaaron,
> 
> Have we grown any clue meanwhile about which Intel boxes need this and for
> which we still need to keep the acpi backlight around?

First of all, there is a bunch of boxes where ACPI backlight works incorrectly
because of the Win8 compatibility issue.  [In short, if we say we are compatible
with Win8, the backlight AML goes into a special code path that is broken on
those machines. Presumably Win8 uses native backlight control on them and that
AML code path is never executed there.]  The collection of machines with this
problem appears to be kind of random (various models from various vendors), but
I *think* they are machines that originally shipped with Win7 with a Win8
"upgrade" option (which in practice requires the BIOS to be updated anyway).

Because of that, last time we tried to switch all of the systems using i915
and telling the BIOS that they are compatible with Win8 over to native backlight
control, but that didn't work for two reasons.  The first reason is that some
user space doesn't know how to use intel_backlight and needs to be taught about
that (so some systems are just not ready for that switch).  The second and more
fundamental reason is that the native backlight control simply doesn't work on
some machines and we don't seem to have any idea why and how to debug this
particular problem (mostly because we don't have enough information and we
don't know what to ask for).

> I've grown _very_ reluctant to just adding tons of quirks to our driver for
> the backlight.
>
> Almost all the quirks we have added recently (or that have been proposed
> to be added) are for the backlight. Imo that indicates we get something
> fundamentally wrong ...

This thing isn't really a quirk.  It rather is an option for the users of
the systems where ACPI backlight doesn't work to switch over to the native
backlight control using a command line switch.  This way they can at least
*see* if the native backlight control works for them and (hopefully) report
problems if that's not the case.  This gives us a chance to get more
information about what the problem is and possibly to make some progress
without making changes for everyone, reverting those changes when they don't
work etc.

An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
make the ACPI backlight work for them again, but this rather is a step back,
because we can't possibly learn anything new from that.

Kind regards,
Rafael


> On Mon, Sep 09, 2013 at 04:42:20PM +0800, Aaron Lu wrote:
> > According to Matthew Garrett, "Windows 8 leaves backlight control up
> > to individual graphics drivers rather than making ACPI calls itself.
> > There's plenty of evidence to suggest that the Intel driver for
> > Windows [8] doesn't use the ACPI interface, including the fact that
> > it's broken on a bunch of machines when the OS claims to support
> > Windows 8.  The simplest thing to do appears to be to disable the
> > ACPI backlight interface on these systems".
> > 
> > There's a problem with that approach, however, because simply
> > avoiding to register the ACPI backlight interface if the firmware
> > calls _OSI for Windows 8 may not work in the following situations:
> >  (1) The ACPI backlight interface actually works on the given system
> >      and the i915 driver is not loaded (e.g. another graphics driver
> >      is used).
> >  (2) The ACPI backlight interface doesn't work on the given system,
> >      but there is a vendor platform driver that will register its
> >      own, equally broken, backlight interface if not prevented from
> >      doing so by the ACPI subsystem.
> > Therefore we need to allow the ACPI backlight interface to be
> > registered until the i915 driver is loaded which then will unregister
> > it if the firmware has called _OSI for Windows 8 (or will register
> > the ACPI video driver without backlight support if not already
> > present).
> > 
> > For this reason, introduce an alternative function for registering
> > ACPI video, __acpi_video_register(bool), that if ture is passed,
> > will check whether or not the ACPI video driver has already been
> > registered and whether or not the backlight Windows 8 quirk has to
> > be applied. If the quirk has to be applied, it will block the ACPI
> > backlight support and either unregister the backlight interface if
> > the ACPI video driver has already been registered, or register the
> > ACPI video driver without the backlight interface otherwise.  Make
> > the i915 driver use __acpi_video_register() instead of
> > acpi_video_register() in i915_driver_load(), and the param passed
> > there is controlled by the i915 module level parameter
> > i915_take_over_backlight, which is set to false by default.
> > 
> > This change is evolved from earlier patches of Matthew Garrett,
> > Chun-Yi Lee and Seth Forshee and is heavily based on two patches
> > from Rafael:
> > https://lkml.org/lkml/2013/7/17/720
> > https://lkml.org/lkml/2013/7/24/806
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > ---
> >  drivers/acpi/internal.h         |  2 ++
> >  drivers/acpi/video.c            | 24 ++++++++++++++++--------
> >  drivers/acpi/video_detect.c     | 15 ++++++++++++++-
> >  drivers/gpu/drm/i915/i915_dma.c |  2 +-
> >  drivers/gpu/drm/i915/i915_drv.c |  5 +++++
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  include/acpi/video.h            |  9 +++++++--
> >  include/linux/acpi.h            |  1 +
> >  8 files changed, 47 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > index 20f4233..a53832e 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev,
> >    -------------------------------------------------------------------------- */
> >  #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
> >  bool acpi_video_backlight_quirks(void);
> > +bool acpi_video_verify_backlight_support(void);
> >  #else
> >  static inline bool acpi_video_backlight_quirks(void) { return false; }
> > +static inline bool acpi_video_verify_backlight_support(void) { return false; }
> >  #endif
> >  
> >  #endif /* _ACPI_INTERNAL_H_ */
> > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> > index 5ad5a71..cc709a7 100644
> > --- a/drivers/acpi/video.c
> > +++ b/drivers/acpi/video.c
> > @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
> >  	unsigned long long level_current, level_next;
> >  	int result = -EINVAL;
> >  
> > -	/* no warning message if acpi_backlight=vendor is used */
> > -	if (!acpi_video_backlight_support())
> > +	/* no warning message if acpi_backlight=vendor or a quirk is used */
> > +	if (!acpi_video_verify_backlight_support())
> >  		return 0;
> >  
> >  	if (!device->brightness)
> > @@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
> >  
> >  static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
> >  {
> > -	if (acpi_video_backlight_support()) {
> > +	if (acpi_video_verify_backlight_support()) {
> >  		struct backlight_properties props;
> >  		struct pci_dev *pdev;
> >  		acpi_handle acpi_parent;
> > @@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void)
> >  	return opregion;
> >  }
> >  
> > -int acpi_video_register(void)
> > +int __acpi_video_register(bool backlight_quirks)
> >  {
> > -	int result = 0;
> > +	bool no_backlight;
> > +	int result;
> > +
> > +	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
> > +
> >  	if (register_count) {
> >  		/*
> > -		 * if the function of acpi_video_register is already called,
> > -		 * don't register the acpi_vide_bus again and return no error.
> > +		 * If acpi_video_register() has been called already, don't try
> > +		 * to register acpi_video_bus, but unregister backlight devices
> > +		 * if no backlight support is requested.
> >  		 */
> > +		if (no_backlight)
> > +			acpi_video_unregister_backlight();
> > +
> >  		return 0;
> >  	}
> >  
> > @@ -1959,7 +1967,7 @@ int acpi_video_register(void)
> >  
> >  	return 0;
> >  }
> > -EXPORT_SYMBOL(acpi_video_register);
> > +EXPORT_SYMBOL(__acpi_video_register);
> >  
> >  void acpi_video_unregister(void)
> >  {
> > diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> > index 940edbf..c7e43ec 100644
> > --- a/drivers/acpi/video_detect.c
> > +++ b/drivers/acpi/video_detect.c
> > @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void)
> >  
> >  bool acpi_video_backlight_quirks(void)
> >  {
> > -	return acpi_gbl_osi_data >= ACPI_OSI_WIN_8;
> > +	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
> > +		acpi_video_caps_check();
> > +		acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT;
> > +		return true;
> > +	}
> > +	return false;
> >  }
> >  EXPORT_SYMBOL(acpi_video_backlight_quirks);
> >  
> > @@ -283,6 +288,14 @@ int acpi_video_backlight_support(void)
> >  }
> >  EXPORT_SYMBOL(acpi_video_backlight_support);
> >  
> > +/* For the ACPI video driver use only. */
> > +bool acpi_video_verify_backlight_support(void)
> > +{
> > +	return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ?
> > +		false : acpi_video_backlight_support();
> > +}
> > +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
> > +
> >  /*
> >   * Use acpi_backlight=vendor/video to force that backlight switching
> >   * is processed by vendor specific acpi drivers or video.ko driver.
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index f466980..75fba17 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	if (INTEL_INFO(dev)->num_pipes) {
> >  		/* Must be done after probing outputs */
> >  		intel_opregion_init(dev);
> > -		acpi_video_register();
> > +		__acpi_video_register(i915_take_over_backlight);
> >  	}
> >  
> >  	if (IS_GEN5(dev))
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 45b3c03..b014398 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -132,6 +132,11 @@ int i915_enable_ips __read_mostly = 1;
> >  module_param_named(enable_ips, i915_enable_ips, int, 0600);
> >  MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
> >  
> > +bool i915_take_over_backlight = false;
> > +module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644);
> > +MODULE_PARM_DESC(take_over_backlight,
> > +		"Prevent ACPI backlight from being used (default: false)");
> > +
> >  static struct drm_driver driver;
> >  extern int intel_agp_enabled;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 1929bff..b43c430 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly;
> >  extern unsigned int i915_preliminary_hw_support __read_mostly;
> >  extern int i915_disable_power_well __read_mostly;
> >  extern int i915_enable_ips __read_mostly;
> > +extern bool i915_take_over_backlight __read_mostly;
> >  
> >  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
> >  extern int i915_resume(struct drm_device *dev);
> > diff --git a/include/acpi/video.h b/include/acpi/video.h
> > index 0c665b5..dc5b8ad 100644
> > --- a/include/acpi/video.h
> > +++ b/include/acpi/video.h
> > @@ -17,13 +17,13 @@ struct acpi_device;
> >  #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
> >  
> >  #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
> > -extern int acpi_video_register(void);
> > +extern int __acpi_video_register(bool backlight_quirks);
> >  extern void acpi_video_unregister(void);
> >  extern int acpi_video_unregister_backlight(void);
> >  extern int acpi_video_get_edid(struct acpi_device *device, int type,
> >  			       int device_id, void **edid);
> >  #else
> > -static inline int acpi_video_register(void) { return 0; }
> > +static inline int __acpi_video_register(bool backlight_quirks) { return 0; }
> >  static inline void acpi_video_unregister(void) { return; }
> >  static inline int acpi_video_unregister_backlight(void) { return; }
> >  static inline int acpi_video_get_edid(struct acpi_device *device, int type,
> > @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
> >  }
> >  #endif
> >  
> > +static inline int acpi_video_register(void)
> > +{
> > +	return __acpi_video_register(false);
> > +}
> > +
> >  #endif
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index a5db4ae..a0dcce5 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid);
> >  #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
> >  #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
> >  #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
> > +#define ACPI_VIDEO_SKIP_BACKLIGHT			0x1000
> >  
> >  #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
> >  
> 
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Daniel Vetter]

On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
> Hi,
> 
> On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
> > Hi Aaaron,
> > 
> > Have we grown any clue meanwhile about which Intel boxes need this and for
> > which we still need to keep the acpi backlight around?
> 
> First of all, there is a bunch of boxes where ACPI backlight works incorrectly
> because of the Win8 compatibility issue.  [In short, if we say we are compatible
> with Win8, the backlight AML goes into a special code path that is broken on
> those machines. Presumably Win8 uses native backlight control on them and that
> AML code path is never executed there.]  The collection of machines with this
> problem appears to be kind of random (various models from various vendors), but
> I *think* they are machines that originally shipped with Win7 with a Win8
> "upgrade" option (which in practice requires the BIOS to be updated anyway).
> 
> Because of that, last time we tried to switch all of the systems using i915
> and telling the BIOS that they are compatible with Win8 over to native backlight
> control, but that didn't work for two reasons.  The first reason is that some
> user space doesn't know how to use intel_backlight and needs to be taught about
> that (so some systems are just not ready for that switch).  The second and more
> fundamental reason is that the native backlight control simply doesn't work on
> some machines and we don't seem to have any idea why and how to debug this
> particular problem (mostly because we don't have enough information and we
> don't know what to ask for).
> 
> > I've grown _very_ reluctant to just adding tons of quirks to our driver for
> > the backlight.
> >
> > Almost all the quirks we have added recently (or that have been proposed
> > to be added) are for the backlight. Imo that indicates we get something
> > fundamentally wrong ...
> 
> This thing isn't really a quirk.  It rather is an option for the users of
> the systems where ACPI backlight doesn't work to switch over to the native
> backlight control using a command line switch.  This way they can at least
> *see* if the native backlight control works for them and (hopefully) report
> problems if that's not the case.  This gives us a chance to get more
> information about what the problem is and possibly to make some progress
> without making changes for everyone, reverting those changes when they don't
> work etc.
> 
> An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
> make the ACPI backlight work for them again, but this rather is a step back,
> because we can't possibly learn anything new from that.

If Win8 is as broken as we are I'm ok with the module option. It just
sounded to me like right now we don't know of a way to make all machines
somewhat happy, combined with the other pile of random backlight issues
the assumption that we do something (maybe something a bit racy) that
windows doesn't do isn't too far-fetched. So I'm not wary of the machines
where the aml is busted for acpi_os=win8, but for the others where this
broke stuff.

Or do I miss something here?

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 919 bytes --]

On Mon, 2013-09-09 at 17:21 +0200, Daniel Vetter wrote:

> If Win8 is as broken as we are I'm ok with the module option. It just
> sounded to me like right now we don't know of a way to make all machines
> somewhat happy, combined with the other pile of random backlight issues
> the assumption that we do something (maybe something a bit racy) that
> windows doesn't do isn't too far-fetched. So I'm not wary of the machines
> where the aml is busted for acpi_os=win8, but for the others where this
> broke stuff.

Windows 8 isn't broken because (as far as we can tell) the Intel drivers
under Windows 8 never use the ACPI backlight set function. Of course, it
would be nice to have that confirmed by Intel.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Rafael J. Wysocki]

On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
> On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
> > > Hi Aaaron,
> > > 
> > > Have we grown any clue meanwhile about which Intel boxes need this and for
> > > which we still need to keep the acpi backlight around?
> > 
> > First of all, there is a bunch of boxes where ACPI backlight works incorrectly
> > because of the Win8 compatibility issue.  [In short, if we say we are compatible
> > with Win8, the backlight AML goes into a special code path that is broken on
> > those machines. Presumably Win8 uses native backlight control on them and that
> > AML code path is never executed there.]  The collection of machines with this
> > problem appears to be kind of random (various models from various vendors), but
> > I *think* they are machines that originally shipped with Win7 with a Win8
> > "upgrade" option (which in practice requires the BIOS to be updated anyway).
> > 
> > Because of that, last time we tried to switch all of the systems using i915
> > and telling the BIOS that they are compatible with Win8 over to native backlight
> > control, but that didn't work for two reasons.  The first reason is that some
> > user space doesn't know how to use intel_backlight and needs to be taught about
> > that (so some systems are just not ready for that switch).  The second and more
> > fundamental reason is that the native backlight control simply doesn't work on
> > some machines and we don't seem to have any idea why and how to debug this
> > particular problem (mostly because we don't have enough information and we
> > don't know what to ask for).
> > 
> > > I've grown _very_ reluctant to just adding tons of quirks to our driver for
> > > the backlight.
> > >
> > > Almost all the quirks we have added recently (or that have been proposed
> > > to be added) are for the backlight. Imo that indicates we get something
> > > fundamentally wrong ...
> > 
> > This thing isn't really a quirk.  It rather is an option for the users of
> > the systems where ACPI backlight doesn't work to switch over to the native
> > backlight control using a command line switch.  This way they can at least
> > *see* if the native backlight control works for them and (hopefully) report
> > problems if that's not the case.  This gives us a chance to get more
> > information about what the problem is and possibly to make some progress
> > without making changes for everyone, reverting those changes when they don't
> > work etc.
> > 
> > An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
> > make the ACPI backlight work for them again, but this rather is a step back,
> > because we can't possibly learn anything new from that.
> 
> If Win8 is as broken as we are I'm ok with the module option. It just
> sounded to me like right now we don't know of a way to make all machines
> somewhat happy, combined with the other pile of random backlight issues
> the assumption that we do something (maybe something a bit racy) that
> windows doesn't do isn't too far-fetched. So I'm not wary of the machines
> where the aml is busted for acpi_os=win8, but for the others where this
> broke stuff.
> 
> Or do I miss something here?

The ACPI video driver doesn't do anything new now.  It only does things that
did work before we started to tell BIOSes we're compatible with Win8.  The
reason why they don't work on some machines now is not related to whether or
not Win8 is broken, but to what is there in the ACPI tables on those machines.
That *is* pure garbage, but Win8 users don't see that (presumably, because
Win8 does't execute the AML in question).  We don't see that either with
acpi_osi="!Windows 2012" (because then we don't execute that AML either).

Whether or not Win8 is broken doesn't matter here.  What matters is that we
have to deal with broken AML somehow.

One way may be to tell everyone affected by this to pass
acpi_osi="!Windows 2012" in the kernel command line or possibly create a
blacklist of those machines in the kernel (which Felipe has been pushing for
recently) and wash our hands clean of this, but that leaves some exceptionally
bad taste in my mouth.

The alternative is to try to use native backlight in i915, which we did, but
that didn't work on some machines.  I don't think we will know why it didn't
work there before we collect some more information and that's not possible
without user testing.  So, the idea is to make that testing possible without
hacking the kernel and that's why we're introducing the new command line
switch.  And we're going to ask users to try it and report back.

Thanks,
Rafael

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Jani Nikula]

On Mon, 09 Sep 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
>> On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
>> > Hi,
>> > 
>> > On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
>> > > Hi Aaaron,
>> > > 
>> > > Have we grown any clue meanwhile about which Intel boxes need this and for
>> > > which we still need to keep the acpi backlight around?
>> > 
>> > First of all, there is a bunch of boxes where ACPI backlight works incorrectly
>> > because of the Win8 compatibility issue.  [In short, if we say we are compatible
>> > with Win8, the backlight AML goes into a special code path that is broken on
>> > those machines. Presumably Win8 uses native backlight control on them and that
>> > AML code path is never executed there.]  The collection of machines with this
>> > problem appears to be kind of random (various models from various vendors), but
>> > I *think* they are machines that originally shipped with Win7 with a Win8
>> > "upgrade" option (which in practice requires the BIOS to be updated anyway).
>> > 
>> > Because of that, last time we tried to switch all of the systems using i915
>> > and telling the BIOS that they are compatible with Win8 over to native backlight
>> > control, but that didn't work for two reasons.  The first reason is that some
>> > user space doesn't know how to use intel_backlight and needs to be taught about
>> > that (so some systems are just not ready for that switch).  The second and more
>> > fundamental reason is that the native backlight control simply doesn't work on
>> > some machines and we don't seem to have any idea why and how to debug this
>> > particular problem (mostly because we don't have enough information and we
>> > don't know what to ask for).
>> > 
>> > > I've grown _very_ reluctant to just adding tons of quirks to our driver for
>> > > the backlight.
>> > >
>> > > Almost all the quirks we have added recently (or that have been proposed
>> > > to be added) are for the backlight. Imo that indicates we get something
>> > > fundamentally wrong ...
>> > 
>> > This thing isn't really a quirk.  It rather is an option for the users of
>> > the systems where ACPI backlight doesn't work to switch over to the native
>> > backlight control using a command line switch.  This way they can at least
>> > *see* if the native backlight control works for them and (hopefully) report
>> > problems if that's not the case.  This gives us a chance to get more
>> > information about what the problem is and possibly to make some progress
>> > without making changes for everyone, reverting those changes when they don't
>> > work etc.
>> > 
>> > An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
>> > make the ACPI backlight work for them again, but this rather is a step back,
>> > because we can't possibly learn anything new from that.
>> 
>> If Win8 is as broken as we are I'm ok with the module option. It just
>> sounded to me like right now we don't know of a way to make all machines
>> somewhat happy, combined with the other pile of random backlight issues
>> the assumption that we do something (maybe something a bit racy) that
>> windows doesn't do isn't too far-fetched. So I'm not wary of the machines
>> where the aml is busted for acpi_os=win8, but for the others where this
>> broke stuff.
>> 
>> Or do I miss something here?
>
> The ACPI video driver doesn't do anything new now.  It only does things that
> did work before we started to tell BIOSes we're compatible with Win8.  The
> reason why they don't work on some machines now is not related to whether or
> not Win8 is broken, but to what is there in the ACPI tables on those machines.
> That *is* pure garbage, but Win8 users don't see that (presumably, because
> Win8 does't execute the AML in question).  We don't see that either with
> acpi_osi="!Windows 2012" (because then we don't execute that AML either).
>
> Whether or not Win8 is broken doesn't matter here.  What matters is that we
> have to deal with broken AML somehow.
>
> One way may be to tell everyone affected by this to pass
> acpi_osi="!Windows 2012" in the kernel command line or possibly create a
> blacklist of those machines in the kernel (which Felipe has been pushing for
> recently) and wash our hands clean of this, but that leaves some exceptionally
> bad taste in my mouth.
>
> The alternative is to try to use native backlight in i915, which we did, but
> that didn't work on some machines.  I don't think we will know why it didn't
> work there before we collect some more information and that's not possible
> without user testing.  So, the idea is to make that testing possible without
> hacking the kernel and that's why we're introducing the new command line
> switch.  And we're going to ask users to try it and report back.

The thing that slightly bugs me with the proposed patches is that
they're adding a module parameter to i915 to tell ACPI video driver
whether to quirk the backlight or not. Before you know, we *will* have
requests to add quirks to i915 to tell ACPI video driver this.

I think the parameter "Does the ACPI backlight interface work or not"
belongs to the ACPI video driver.

Feel free to file this in your bikeshedding folder, but I think i915
should only tell ACPI "I have a native backlight interface". What ACPI
video driver does with that information is its business. The desired
thing to do here would be to check if there's a module parameter or a
quirk to disable the ACPI backlight interface. acpi_backlight=DTRT? ;)

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 665 bytes --]

On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote:

> I think the parameter "Does the ACPI backlight interface work or not"
> belongs to the ACPI video driver.

That depends on how Windows 8 works. If Windows 8 policy is handled by
the GPU drivers then it belongs in i915. If it's handled by the ACPI
code then it belongs in the ACPI code. But I have no way of determining
that, whereas you work for a company that produces a Windows 8 video
driver…

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Jani Nikula]

On Tue, 10 Sep 2013, Matthew Garrett <matthew.garrett@nebula.com> wrote:
> On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote:
>
>> I think the parameter "Does the ACPI backlight interface work or not"
>> belongs to the ACPI video driver.
>
> That depends on how Windows 8 works. If Windows 8 policy is handled by
> the GPU drivers then it belongs in i915. If it's handled by the ACPI
> code then it belongs in the ACPI code.

I fail to see the logic. Windows 8 policy dictates whether we can use
the AML code or not. IMHO, ACPI code is in the best position to figure
this out and quirk as necessary. It's the part that knows about Windows
8, not i915.

> But I have no way of determining that, whereas you work for a company
> that produces a Windows 8 video driver…

Here I do see the logic. However see signature; I'm not in the best of
positions to figure out things about Windows 8 video drivers. ;) But
I'll see what I can do.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1058 bytes --]

On Tue, 2013-09-10 at 17:21 +0300, Jani Nikula wrote:
> On Tue, 10 Sep 2013, Matthew Garrett <matthew.garrett@nebula.com> wrote:
> > On Tue, 2013-09-10 at 16:53 +0300, Jani Nikula wrote:
> >
> >> I think the parameter "Does the ACPI backlight interface work or not"
> >> belongs to the ACPI video driver.
> >
> > That depends on how Windows 8 works. If Windows 8 policy is handled by
> > the GPU drivers then it belongs in i915. If it's handled by the ACPI
> > code then it belongs in the ACPI code.
> 
> I fail to see the logic. Windows 8 policy dictates whether we can use
> the AML code or not. IMHO, ACPI code is in the best position to figure
> this out and quirk as necessary. It's the part that knows about Windows
> 8, not i915.

So if nvidia hardware uses the ACPI interface and Intel doesn't, we
should still quirk it in the ACPI driver?

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Rafael J. Wysocki]

On Tuesday, September 10, 2013 04:53:40 PM Jani Nikula wrote:
> On Mon, 09 Sep 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
> >> On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
> >> > Hi,
> >> > 
> >> > On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
> >> > > Hi Aaaron,
> >> > > 
> >> > > Have we grown any clue meanwhile about which Intel boxes need this and for
> >> > > which we still need to keep the acpi backlight around?
> >> > 
> >> > First of all, there is a bunch of boxes where ACPI backlight works incorrectly
> >> > because of the Win8 compatibility issue.  [In short, if we say we are compatible
> >> > with Win8, the backlight AML goes into a special code path that is broken on
> >> > those machines. Presumably Win8 uses native backlight control on them and that
> >> > AML code path is never executed there.]  The collection of machines with this
> >> > problem appears to be kind of random (various models from various vendors), but
> >> > I *think* they are machines that originally shipped with Win7 with a Win8
> >> > "upgrade" option (which in practice requires the BIOS to be updated anyway).
> >> > 
> >> > Because of that, last time we tried to switch all of the systems using i915
> >> > and telling the BIOS that they are compatible with Win8 over to native backlight
> >> > control, but that didn't work for two reasons.  The first reason is that some
> >> > user space doesn't know how to use intel_backlight and needs to be taught about
> >> > that (so some systems are just not ready for that switch).  The second and more
> >> > fundamental reason is that the native backlight control simply doesn't work on
> >> > some machines and we don't seem to have any idea why and how to debug this
> >> > particular problem (mostly because we don't have enough information and we
> >> > don't know what to ask for).
> >> > 
> >> > > I've grown _very_ reluctant to just adding tons of quirks to our driver for
> >> > > the backlight.
> >> > >
> >> > > Almost all the quirks we have added recently (or that have been proposed
> >> > > to be added) are for the backlight. Imo that indicates we get something
> >> > > fundamentally wrong ...
> >> > 
> >> > This thing isn't really a quirk.  It rather is an option for the users of
> >> > the systems where ACPI backlight doesn't work to switch over to the native
> >> > backlight control using a command line switch.  This way they can at least
> >> > *see* if the native backlight control works for them and (hopefully) report
> >> > problems if that's not the case.  This gives us a chance to get more
> >> > information about what the problem is and possibly to make some progress
> >> > without making changes for everyone, reverting those changes when they don't
> >> > work etc.
> >> > 
> >> > An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
> >> > make the ACPI backlight work for them again, but this rather is a step back,
> >> > because we can't possibly learn anything new from that.
> >> 
> >> If Win8 is as broken as we are I'm ok with the module option. It just
> >> sounded to me like right now we don't know of a way to make all machines
> >> somewhat happy, combined with the other pile of random backlight issues
> >> the assumption that we do something (maybe something a bit racy) that
> >> windows doesn't do isn't too far-fetched. So I'm not wary of the machines
> >> where the aml is busted for acpi_os=win8, but for the others where this
> >> broke stuff.
> >> 
> >> Or do I miss something here?
> >
> > The ACPI video driver doesn't do anything new now.  It only does things that
> > did work before we started to tell BIOSes we're compatible with Win8.  The
> > reason why they don't work on some machines now is not related to whether or
> > not Win8 is broken, but to what is there in the ACPI tables on those machines.
> > That *is* pure garbage, but Win8 users don't see that (presumably, because
> > Win8 does't execute the AML in question).  We don't see that either with
> > acpi_osi="!Windows 2012" (because then we don't execute that AML either).
> >
> > Whether or not Win8 is broken doesn't matter here.  What matters is that we
> > have to deal with broken AML somehow.
> >
> > One way may be to tell everyone affected by this to pass
> > acpi_osi="!Windows 2012" in the kernel command line or possibly create a
> > blacklist of those machines in the kernel (which Felipe has been pushing for
> > recently) and wash our hands clean of this, but that leaves some exceptionally
> > bad taste in my mouth.
> >
> > The alternative is to try to use native backlight in i915, which we did, but
> > that didn't work on some machines.  I don't think we will know why it didn't
> > work there before we collect some more information and that's not possible
> > without user testing.  So, the idea is to make that testing possible without
> > hacking the kernel and that's why we're introducing the new command line
> > switch.  And we're going to ask users to try it and report back.
> 
> The thing that slightly bugs me with the proposed patches is that
> they're adding a module parameter to i915 to tell ACPI video driver
> whether to quirk the backlight or not. Before you know, we *will* have
> requests to add quirks to i915 to tell ACPI video driver this.
> 
> I think the parameter "Does the ACPI backlight interface work or not"
> belongs to the ACPI video driver.
>
> Feel free to file this in your bikeshedding folder, but I think i915
> should only tell ACPI "I have a native backlight interface".

It kind of does that already through the intel_opregion_init() thing,
so it would be trivial to test intel_opregion_present() in
acpi_video_register().

So yes, we can add a command line option, say 'use_native_backlight' to the
ACPI video driver that will work like this: "if the Intel opregion is present
and video.use_native_backlight is set, skip registering ACPI backlight".

Is that what you want?

Rafael

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Aaron Lu]

On Tue, Sep 10, 2013 at 09:23:04PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 10, 2013 04:53:40 PM Jani Nikula wrote:
> > On Mon, 09 Sep 2013, "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > > On Monday, September 09, 2013 05:21:18 PM Daniel Vetter wrote:
> > >> On Mon, Sep 09, 2013 at 02:16:12PM +0200, Rafael J. Wysocki wrote:
> > >> > Hi,
> > >> > 
> > >> > On Monday, September 09, 2013 11:32:10 AM Daniel Vetter wrote:
> > >> > > Hi Aaaron,
> > >> > > 
> > >> > > Have we grown any clue meanwhile about which Intel boxes need this and for
> > >> > > which we still need to keep the acpi backlight around?
> > >> > 
> > >> > First of all, there is a bunch of boxes where ACPI backlight works incorrectly
> > >> > because of the Win8 compatibility issue.  [In short, if we say we are compatible
> > >> > with Win8, the backlight AML goes into a special code path that is broken on
> > >> > those machines. Presumably Win8 uses native backlight control on them and that
> > >> > AML code path is never executed there.]  The collection of machines with this
> > >> > problem appears to be kind of random (various models from various vendors), but
> > >> > I *think* they are machines that originally shipped with Win7 with a Win8
> > >> > "upgrade" option (which in practice requires the BIOS to be updated anyway).
> > >> > 
> > >> > Because of that, last time we tried to switch all of the systems using i915
> > >> > and telling the BIOS that they are compatible with Win8 over to native backlight
> > >> > control, but that didn't work for two reasons.  The first reason is that some
> > >> > user space doesn't know how to use intel_backlight and needs to be taught about
> > >> > that (so some systems are just not ready for that switch).  The second and more
> > >> > fundamental reason is that the native backlight control simply doesn't work on
> > >> > some machines and we don't seem to have any idea why and how to debug this
> > >> > particular problem (mostly because we don't have enough information and we
> > >> > don't know what to ask for).
> > >> > 
> > >> > > I've grown _very_ reluctant to just adding tons of quirks to our driver for
> > >> > > the backlight.
> > >> > >
> > >> > > Almost all the quirks we have added recently (or that have been proposed
> > >> > > to be added) are for the backlight. Imo that indicates we get something
> > >> > > fundamentally wrong ...
> > >> > 
> > >> > This thing isn't really a quirk.  It rather is an option for the users of
> > >> > the systems where ACPI backlight doesn't work to switch over to the native
> > >> > backlight control using a command line switch.  This way they can at least
> > >> > *see* if the native backlight control works for them and (hopefully) report
> > >> > problems if that's not the case.  This gives us a chance to get more
> > >> > information about what the problem is and possibly to make some progress
> > >> > without making changes for everyone, reverting those changes when they don't
> > >> > work etc.
> > >> > 
> > >> > An alternative for them is to pass acpi_osi="!Windows 2012" which will probably
> > >> > make the ACPI backlight work for them again, but this rather is a step back,
> > >> > because we can't possibly learn anything new from that.
> > >> 
> > >> If Win8 is as broken as we are I'm ok with the module option. It just
> > >> sounded to me like right now we don't know of a way to make all machines
> > >> somewhat happy, combined with the other pile of random backlight issues
> > >> the assumption that we do something (maybe something a bit racy) that
> > >> windows doesn't do isn't too far-fetched. So I'm not wary of the machines
> > >> where the aml is busted for acpi_os=win8, but for the others where this
> > >> broke stuff.
> > >> 
> > >> Or do I miss something here?
> > >
> > > The ACPI video driver doesn't do anything new now.  It only does things that
> > > did work before we started to tell BIOSes we're compatible with Win8.  The
> > > reason why they don't work on some machines now is not related to whether or
> > > not Win8 is broken, but to what is there in the ACPI tables on those machines.
> > > That *is* pure garbage, but Win8 users don't see that (presumably, because
> > > Win8 does't execute the AML in question).  We don't see that either with
> > > acpi_osi="!Windows 2012" (because then we don't execute that AML either).
> > >
> > > Whether or not Win8 is broken doesn't matter here.  What matters is that we
> > > have to deal with broken AML somehow.
> > >
> > > One way may be to tell everyone affected by this to pass
> > > acpi_osi="!Windows 2012" in the kernel command line or possibly create a
> > > blacklist of those machines in the kernel (which Felipe has been pushing for
> > > recently) and wash our hands clean of this, but that leaves some exceptionally
> > > bad taste in my mouth.
> > >
> > > The alternative is to try to use native backlight in i915, which we did, but
> > > that didn't work on some machines.  I don't think we will know why it didn't
> > > work there before we collect some more information and that's not possible
> > > without user testing.  So, the idea is to make that testing possible without
> > > hacking the kernel and that's why we're introducing the new command line
> > > switch.  And we're going to ask users to try it and report back.
> > 
> > The thing that slightly bugs me with the proposed patches is that
> > they're adding a module parameter to i915 to tell ACPI video driver
> > whether to quirk the backlight or not. Before you know, we *will* have
> > requests to add quirks to i915 to tell ACPI video driver this.
> > 
> > I think the parameter "Does the ACPI backlight interface work or not"
> > belongs to the ACPI video driver.
> >
> > Feel free to file this in your bikeshedding folder, but I think i915
> > should only tell ACPI "I have a native backlight interface".
> 
> It kind of does that already through the intel_opregion_init() thing,
> so it would be trivial to test intel_opregion_present() in
> acpi_video_register().

It is possible the i915 driver decides not to register a backlight
interface for the graphics card for some reason(memory allocation failed
or it knows the native control does not work on this card or whatever),
so I would prefer let i915 tell ACPI video that it has registered a
native backlight control interface as Jani has said.

Then together with the video.use_native_backlight, we can register or
not register ACPI video backlight interface accordingly. Or rather, we
can simply not register ACPI video backlight interface for Win8 systems
as long as i915 indicates that it has native backlight control(if the
native control is broken, i915 should fix it or blacklist it so that
i915 will not indicate it has native backlight control and ACPI video
will continue to register its own).

How does this sound?

-Aaron

> 
> So yes, we can add a command line option, say 'use_native_backlight' to the
> ACPI video driver that will work like this: "if the Intel opregion is present
> and video.use_native_backlight is set, skip registering ACPI backlight".
> 
> Is that what you want?
> 
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Jani Nikula]

On Wed, 11 Sep 2013, Aaron Lu <aaron.lu@intel.com> wrote:
> It is possible the i915 driver decides not to register a backlight
> interface for the graphics card for some reason(memory allocation failed
> or it knows the native control does not work on this card or whatever),
> so I would prefer let i915 tell ACPI video that it has registered a
> native backlight control interface as Jani has said.
>
> Then together with the video.use_native_backlight, we can register or
> not register ACPI video backlight interface accordingly. Or rather, we
> can simply not register ACPI video backlight interface for Win8 systems
> as long as i915 indicates that it has native backlight control(if the
> native control is broken, i915 should fix it or blacklist it so that
> i915 will not indicate it has native backlight control and ACPI video
> will continue to register its own).
>
> How does this sound?

Sounds good to me.

Before plunging forward, have you observed any difference between the
boot modes? We have reports [1] that the backlight behaviour is
different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
story.

Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
paths, what guarantees do we have of UEFI+CSM or legacy boots working?

BR,
Jani.


[1] https://bugzilla.kernel.org/show_bug.cgi?id=47941#c96


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 797 bytes --]

On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:

> Before plunging forward, have you observed any difference between the
> boot modes? We have reports [1] that the backlight behaviour is
> different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
> acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
> story.
> 
> Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
> paths, what guarantees do we have of UEFI+CSM or legacy boots working?

We have no evidence of Windows behaving differently based on the exposed
firmware type.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Yves-Alexis Perez]

On mer., 2013-09-11 at 08:45 +0000, Matthew Garrett wrote:
> On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:
> 
> > Before plunging forward, have you observed any difference between the
> > boot modes? We have reports [1] that the backlight behaviour is
> > different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
> > acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
> > story.
> > 
> > Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
> > paths, what guarantees do we have of UEFI+CSM or legacy boots working?
> 
> We have no evidence of Windows behaving differently based on the exposed
> firmware type.
> 
I do have an X230 which boots fine using pure UEFI or UEFI+CSM (and I
guess I can try booting it with a grml for pure legacy), so I can do
tests if needed.

Regards,
-- 
Yves-Alexis

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Jani Nikula]

On Wed, 11 Sep 2013, Matthew Garrett <matthew.garrett@nebula.com> wrote:
> On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:
>
>> Before plunging forward, have you observed any difference between the
>> boot modes? We have reports [1] that the backlight behaviour is
>> different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
>> acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
>> story.
>> 
>> Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
>> paths, what guarantees do we have of UEFI+CSM or legacy boots working?
>
> We have no evidence of Windows behaving differently based on the exposed
> firmware type.

By "behaving differently", do you mean internally adapting to the boot
mode, or exhibiting different behaviour to the user?

We have evidence of the firmware behaving differently (VBT, backlight)
based on the boot mode, all else being equal. We don't adapt to that,
and we fail. I don't know if we should adapt, or do things differently
altogether. I don't even know if Windows 8 works on all boot modes on
the machines in question.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1571 bytes --]

On Wed, 2013-09-11 at 13:29 +0300, Jani Nikula wrote:
> On Wed, 11 Sep 2013, Matthew Garrett <matthew.garrett@nebula.com> wrote:
> > On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:
> >
> >> Before plunging forward, have you observed any difference between the
> >> boot modes? We have reports [1] that the backlight behaviour is
> >> different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
> >> acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
> >> story.
> >> 
> >> Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
> >> paths, what guarantees do we have of UEFI+CSM or legacy boots working?
> >
> > We have no evidence of Windows behaving differently based on the exposed
> > firmware type.
> 
> By "behaving differently", do you mean internally adapting to the boot
> mode, or exhibiting different behaviour to the user?

As far as backlight control goes, both.

> We have evidence of the firmware behaving differently (VBT, backlight)
> based on the boot mode, all else being equal. We don't adapt to that,
> and we fail. I don't know if we should adapt, or do things differently
> altogether. I don't even know if Windows 8 works on all boot modes on
> the machines in question.

Sure, but Windows knows nothing about VBT or opregion-backed backlight
control. That's up to the Intel driver.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Aaron Lu]

On Wed, Sep 11, 2013 at 11:45:19AM +0300, Jani Nikula wrote:
> On Wed, 11 Sep 2013, Aaron Lu <aaron.lu@intel.com> wrote:
> > It is possible the i915 driver decides not to register a backlight
> > interface for the graphics card for some reason(memory allocation failed
> > or it knows the native control does not work on this card or whatever),
> > so I would prefer let i915 tell ACPI video that it has registered a
> > native backlight control interface as Jani has said.
> >
> > Then together with the video.use_native_backlight, we can register or
> > not register ACPI video backlight interface accordingly. Or rather, we
> > can simply not register ACPI video backlight interface for Win8 systems
> > as long as i915 indicates that it has native backlight control(if the
> > native control is broken, i915 should fix it or blacklist it so that
> > i915 will not indicate it has native backlight control and ACPI video
> > will continue to register its own).
> >
> > How does this sound?
> 
> Sounds good to me.
> 
> Before plunging forward, have you observed any difference between the
> boot modes? We have reports [1] that the backlight behaviour is

Not yet from ACPI's point of view.

> different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
> acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
> story.

This check in patch 2/2 is a policy: for Win8 system, we think the
native backlight control has a better chance of working than the ACPI
video's, so I think the check is enough in ACPI video.

> 
> Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
> paths, what guarantees do we have of UEFI+CSM or legacy boots working?

I suppose the 'tested BIOS code paths' means the pure UEFI boot mode? I
don't know what guatantees do we have since I don't know what happened
underneath after the backlight register is set in i915 driver, you or
other i915 driver people should know more than I do :-)

BTW, after the backlight register is set in i915, is it that some
find of firmware code will run in response to the setting of the
register(e.g. the BLC_PWM_CTL/BLC_PWM_CPU_CTL/PCI_LBPC reg)?

Thanks,
Aaron

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Aaron Lu]

On Mon, Sep 09, 2013 at 11:32:10AM +0200, Daniel Vetter wrote:
> Hi Aaaron,
> 
> Have we grown any clue meanwhile about which Intel boxes need this and for
> which we still need to keep the acpi backlight around? I've grown _very_

Sorry, no general rule has been found. As Rafael has said, it appears
to be random... And in addition to the "shipped with Win7 with a Win8
upgrade option" case, I also find a Sony laptop that has Win8
pre-installed and a broken ACPI video backlight interface. Its ACPI
video backlight control method is simply a stub, so even the
acpi_osi="!Windows 2012" will not provide a working backlight for this
system.

-Aaron

> reluctant to just adding tons of quirks to our driver for the backlight.
> Almost all the quirks we have added recently (or that have been proposed
> to be added) are for the backlight. Imo that indicates we get something
> fundamentally wrong ...
> 
> Cheers, Daniel
> 
> On Mon, Sep 09, 2013 at 04:42:20PM +0800, Aaron Lu wrote:
> > According to Matthew Garrett, "Windows 8 leaves backlight control up
> > to individual graphics drivers rather than making ACPI calls itself.
> > There's plenty of evidence to suggest that the Intel driver for
> > Windows [8] doesn't use the ACPI interface, including the fact that
> > it's broken on a bunch of machines when the OS claims to support
> > Windows 8.  The simplest thing to do appears to be to disable the
> > ACPI backlight interface on these systems".
> > 
> > There's a problem with that approach, however, because simply
> > avoiding to register the ACPI backlight interface if the firmware
> > calls _OSI for Windows 8 may not work in the following situations:
> >  (1) The ACPI backlight interface actually works on the given system
> >      and the i915 driver is not loaded (e.g. another graphics driver
> >      is used).
> >  (2) The ACPI backlight interface doesn't work on the given system,
> >      but there is a vendor platform driver that will register its
> >      own, equally broken, backlight interface if not prevented from
> >      doing so by the ACPI subsystem.
> > Therefore we need to allow the ACPI backlight interface to be
> > registered until the i915 driver is loaded which then will unregister
> > it if the firmware has called _OSI for Windows 8 (or will register
> > the ACPI video driver without backlight support if not already
> > present).
> > 
> > For this reason, introduce an alternative function for registering
> > ACPI video, __acpi_video_register(bool), that if ture is passed,
> > will check whether or not the ACPI video driver has already been
> > registered and whether or not the backlight Windows 8 quirk has to
> > be applied. If the quirk has to be applied, it will block the ACPI
> > backlight support and either unregister the backlight interface if
> > the ACPI video driver has already been registered, or register the
> > ACPI video driver without the backlight interface otherwise.  Make
> > the i915 driver use __acpi_video_register() instead of
> > acpi_video_register() in i915_driver_load(), and the param passed
> > there is controlled by the i915 module level parameter
> > i915_take_over_backlight, which is set to false by default.
> > 
> > This change is evolved from earlier patches of Matthew Garrett,
> > Chun-Yi Lee and Seth Forshee and is heavily based on two patches
> > from Rafael:
> > https://lkml.org/lkml/2013/7/17/720
> > https://lkml.org/lkml/2013/7/24/806
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > ---
> >  drivers/acpi/internal.h         |  2 ++
> >  drivers/acpi/video.c            | 24 ++++++++++++++++--------
> >  drivers/acpi/video_detect.c     | 15 ++++++++++++++-
> >  drivers/gpu/drm/i915/i915_dma.c |  2 +-
> >  drivers/gpu/drm/i915/i915_drv.c |  5 +++++
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  include/acpi/video.h            |  9 +++++++--
> >  include/linux/acpi.h            |  1 +
> >  8 files changed, 47 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > index 20f4233..a53832e 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -170,8 +170,10 @@ int acpi_create_platform_device(struct acpi_device *adev,
> >    -------------------------------------------------------------------------- */
> >  #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
> >  bool acpi_video_backlight_quirks(void);
> > +bool acpi_video_verify_backlight_support(void);
> >  #else
> >  static inline bool acpi_video_backlight_quirks(void) { return false; }
> > +static inline bool acpi_video_verify_backlight_support(void) { return false; }
> >  #endif
> >  
> >  #endif /* _ACPI_INTERNAL_H_ */
> > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> > index 5ad5a71..cc709a7 100644
> > --- a/drivers/acpi/video.c
> > +++ b/drivers/acpi/video.c
> > @@ -1256,8 +1256,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
> >  	unsigned long long level_current, level_next;
> >  	int result = -EINVAL;
> >  
> > -	/* no warning message if acpi_backlight=vendor is used */
> > -	if (!acpi_video_backlight_support())
> > +	/* no warning message if acpi_backlight=vendor or a quirk is used */
> > +	if (!acpi_video_verify_backlight_support())
> >  		return 0;
> >  
> >  	if (!device->brightness)
> > @@ -1558,7 +1558,7 @@ acpi_video_bus_match(acpi_handle handle, u32 level, void *context,
> >  
> >  static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
> >  {
> > -	if (acpi_video_backlight_support()) {
> > +	if (acpi_video_verify_backlight_support()) {
> >  		struct backlight_properties props;
> >  		struct pci_dev *pdev;
> >  		acpi_handle acpi_parent;
> > @@ -1933,14 +1933,22 @@ static int __init intel_opregion_present(void)
> >  	return opregion;
> >  }
> >  
> > -int acpi_video_register(void)
> > +int __acpi_video_register(bool backlight_quirks)
> >  {
> > -	int result = 0;
> > +	bool no_backlight;
> > +	int result;
> > +
> > +	no_backlight = backlight_quirks ? acpi_video_backlight_quirks() : false;
> > +
> >  	if (register_count) {
> >  		/*
> > -		 * if the function of acpi_video_register is already called,
> > -		 * don't register the acpi_vide_bus again and return no error.
> > +		 * If acpi_video_register() has been called already, don't try
> > +		 * to register acpi_video_bus, but unregister backlight devices
> > +		 * if no backlight support is requested.
> >  		 */
> > +		if (no_backlight)
> > +			acpi_video_unregister_backlight();
> > +
> >  		return 0;
> >  	}
> >  
> > @@ -1959,7 +1967,7 @@ int acpi_video_register(void)
> >  
> >  	return 0;
> >  }
> > -EXPORT_SYMBOL(acpi_video_register);
> > +EXPORT_SYMBOL(__acpi_video_register);
> >  
> >  void acpi_video_unregister(void)
> >  {
> > diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> > index 940edbf..c7e43ec 100644
> > --- a/drivers/acpi/video_detect.c
> > +++ b/drivers/acpi/video_detect.c
> > @@ -235,7 +235,12 @@ static void acpi_video_caps_check(void)
> >  
> >  bool acpi_video_backlight_quirks(void)
> >  {
> > -	return acpi_gbl_osi_data >= ACPI_OSI_WIN_8;
> > +	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_8) {
> > +		acpi_video_caps_check();
> > +		acpi_video_support |= ACPI_VIDEO_SKIP_BACKLIGHT;
> > +		return true;
> > +	}
> > +	return false;
> >  }
> >  EXPORT_SYMBOL(acpi_video_backlight_quirks);
> >  
> > @@ -283,6 +288,14 @@ int acpi_video_backlight_support(void)
> >  }
> >  EXPORT_SYMBOL(acpi_video_backlight_support);
> >  
> > +/* For the ACPI video driver use only. */
> > +bool acpi_video_verify_backlight_support(void)
> > +{
> > +	return (acpi_video_support & ACPI_VIDEO_SKIP_BACKLIGHT) ?
> > +		false : acpi_video_backlight_support();
> > +}
> > +EXPORT_SYMBOL(acpi_video_verify_backlight_support);
> > +
> >  /*
> >   * Use acpi_backlight=vendor/video to force that backlight switching
> >   * is processed by vendor specific acpi drivers or video.ko driver.
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index f466980..75fba17 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	if (INTEL_INFO(dev)->num_pipes) {
> >  		/* Must be done after probing outputs */
> >  		intel_opregion_init(dev);
> > -		acpi_video_register();
> > +		__acpi_video_register(i915_take_over_backlight);
> >  	}
> >  
> >  	if (IS_GEN5(dev))
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 45b3c03..b014398 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -132,6 +132,11 @@ int i915_enable_ips __read_mostly = 1;
> >  module_param_named(enable_ips, i915_enable_ips, int, 0600);
> >  MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)");
> >  
> > +bool i915_take_over_backlight = false;
> > +module_param_named(take_over_backlight, i915_take_over_backlight, bool, 0644);
> > +MODULE_PARM_DESC(take_over_backlight,
> > +		"Prevent ACPI backlight from being used (default: false)");
> > +
> >  static struct drm_driver driver;
> >  extern int intel_agp_enabled;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 1929bff..b43c430 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1543,6 +1543,7 @@ extern int i915_enable_ppgtt __read_mostly;
> >  extern unsigned int i915_preliminary_hw_support __read_mostly;
> >  extern int i915_disable_power_well __read_mostly;
> >  extern int i915_enable_ips __read_mostly;
> > +extern bool i915_take_over_backlight __read_mostly;
> >  
> >  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
> >  extern int i915_resume(struct drm_device *dev);
> > diff --git a/include/acpi/video.h b/include/acpi/video.h
> > index 0c665b5..dc5b8ad 100644
> > --- a/include/acpi/video.h
> > +++ b/include/acpi/video.h
> > @@ -17,13 +17,13 @@ struct acpi_device;
> >  #define ACPI_VIDEO_DISPLAY_LEGACY_TV      0x0200
> >  
> >  #if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
> > -extern int acpi_video_register(void);
> > +extern int __acpi_video_register(bool backlight_quirks);
> >  extern void acpi_video_unregister(void);
> >  extern int acpi_video_unregister_backlight(void);
> >  extern int acpi_video_get_edid(struct acpi_device *device, int type,
> >  			       int device_id, void **edid);
> >  #else
> > -static inline int acpi_video_register(void) { return 0; }
> > +static inline int __acpi_video_register(bool backlight_quirks) { return 0; }
> >  static inline void acpi_video_unregister(void) { return; }
> >  static inline int acpi_video_unregister_backlight(void) { return; }
> >  static inline int acpi_video_get_edid(struct acpi_device *device, int type,
> > @@ -33,4 +33,9 @@ static inline int acpi_video_get_edid(struct acpi_device *device, int type,
> >  }
> >  #endif
> >  
> > +static inline int acpi_video_register(void)
> > +{
> > +	return __acpi_video_register(false);
> > +}
> > +
> >  #endif
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index a5db4ae..a0dcce5 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -191,6 +191,7 @@ extern bool wmi_has_guid(const char *guid);
> >  #define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
> >  #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
> >  #define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
> > +#define ACPI_VIDEO_SKIP_BACKLIGHT			0x1000
> >  
> >  #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
> >  
> > -- 
> > 1.8.4.12.g2ea3df6
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Igor Gnatenko]

On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index f466980..75fba17 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	if (INTEL_INFO(dev)->num_pipes) {
>  		/* Must be done after probing outputs */
>  		intel_opregion_init(dev);
> -		acpi_video_register();
> +		__acpi_video_register(i915_take_over_backlight);
>  	}
>  
>  	if (IS_GEN5(dev))

I can't compile:


DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load':
DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit
declaration of function
'__acpi_video_register' [-Werror=implicit-function-declaration]
DEBUG:    __acpi_video_register(i915_take_over_backlight);
DEBUG:    ^
DEBUG: cc1: some warnings being treated as errors
DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1
DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2
DEBUG: make[2]: *** [drivers/gpu/drm] Error 2
DEBUG: make[1]: *** [drivers/gpu] Error 2
DEBUG: make: *** [drivers] Error 2

-- 
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.0-3.fc20.x86_64

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Aaron Lu]

On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
> On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index f466980..75fba17 100644
>> --- a/drivers/gpu/drm/i915/i915_dma.c
>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>  	if (INTEL_INFO(dev)->num_pipes) {
>>  		/* Must be done after probing outputs */
>>  		intel_opregion_init(dev);
>> -		acpi_video_register();
>> +		__acpi_video_register(i915_take_over_backlight);
>>  	}
>>  
>>  	if (IS_GEN5(dev))
> 
> I can't compile:
> 
> 
> DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load':
> DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit
> declaration of function
> '__acpi_video_register' [-Werror=implicit-function-declaration]
> DEBUG:    __acpi_video_register(i915_take_over_backlight);
> DEBUG:    ^
> DEBUG: cc1: some warnings being treated as errors
> DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1
> DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2
> DEBUG: make[2]: *** [drivers/gpu/drm] Error 2
> DEBUG: make[1]: *** [drivers/gpu] Error 2
> DEBUG: make: *** [drivers] Error 2
> 

The two patches are based on top of Rafael's linux-next tree. I just
tried it again, no compile problem for me. I also tried on today Linus'
master tree, as there are some updates from i915, two conflicts exist.
I've just resolved them and will update it in next revision.
If you want to try it now, please use:
https://github.com/aaronlu/linux acpi_video_rework

Thanks,
Aaron

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Igor Gnatenko]

On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote:
> On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
> > On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
> >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> >> index f466980..75fba17 100644
> >> --- a/drivers/gpu/drm/i915/i915_dma.c
> >> +++ b/drivers/gpu/drm/i915/i915_dma.c
> >> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >>  	if (INTEL_INFO(dev)->num_pipes) {
> >>  		/* Must be done after probing outputs */
> >>  		intel_opregion_init(dev);
> >> -		acpi_video_register();
> >> +		__acpi_video_register(i915_take_over_backlight);
> >>  	}
> >>  
> >>  	if (IS_GEN5(dev))
> > 
> > I can't compile:
> > 
> > 
> > DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load':
> > DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit
> > declaration of function
> > '__acpi_video_register' [-Werror=implicit-function-declaration]
> > DEBUG:    __acpi_video_register(i915_take_over_backlight);
> > DEBUG:    ^
> > DEBUG: cc1: some warnings being treated as errors
> > DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1
> > DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2
> > DEBUG: make[2]: *** [drivers/gpu/drm] Error 2
> > DEBUG: make[1]: *** [drivers/gpu] Error 2
> > DEBUG: make: *** [drivers] Error 2
> > 
> 
> The two patches are based on top of Rafael's linux-next tree. I just
> tried it again, no compile problem for me. I also tried on today Linus'
> master tree, as there are some updates from i915, two conflicts exist.
> I've just resolved them and will update it in next revision.
> If you want to try it now, please use:
> https://github.com/aaronlu/linux acpi_video_rework
> 
> Thanks,
> Aaron

Thanks. this patch fixes my problems w/ compilation. I've tested this
two patches and after apply I have:
$ tree /sys/class/backlight/
/sys/class/backlight/
|-- acpi_video0
-> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0
`-- intel_backlight
-> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight

2 directories, 0 files

I think it's didn't unregistered.. I may forget. I need to apply one of
patch from Matthew ?

Some strings from logs:
DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013
thinkpad_acpi: Standard ACPI backlight interface available, not loading
native one
-- 
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.0-1.fc20.x86_64

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Aaron Lu]

On 09/10/2013 01:13 PM, Igor Gnatenko wrote:
> On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote:
>> On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
>>> On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
>>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>>>> index f466980..75fba17 100644
>>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>>> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>>>  	if (INTEL_INFO(dev)->num_pipes) {
>>>>  		/* Must be done after probing outputs */
>>>>  		intel_opregion_init(dev);
>>>> -		acpi_video_register();
>>>> +		__acpi_video_register(i915_take_over_backlight);
>>>>  	}
>>>>  
>>>>  	if (IS_GEN5(dev))
>>>
>>> I can't compile:
>>>
>>>
>>> DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load':
>>> DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit
>>> declaration of function
>>> '__acpi_video_register' [-Werror=implicit-function-declaration]
>>> DEBUG:    __acpi_video_register(i915_take_over_backlight);
>>> DEBUG:    ^
>>> DEBUG: cc1: some warnings being treated as errors
>>> DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1
>>> DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2
>>> DEBUG: make[2]: *** [drivers/gpu/drm] Error 2
>>> DEBUG: make[1]: *** [drivers/gpu] Error 2
>>> DEBUG: make: *** [drivers] Error 2
>>>
>>
>> The two patches are based on top of Rafael's linux-next tree. I just
>> tried it again, no compile problem for me. I also tried on today Linus'
>> master tree, as there are some updates from i915, two conflicts exist.
>> I've just resolved them and will update it in next revision.
>> If you want to try it now, please use:
>> https://github.com/aaronlu/linux acpi_video_rework
>>
>> Thanks,
>> Aaron
> 
> Thanks. this patch fixes my problems w/ compilation. I've tested this
> two patches and after apply I have:
> $ tree /sys/class/backlight/
> /sys/class/backlight/
> |-- acpi_video0
> -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0
> `-- intel_backlight
> -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
> 
> 2 directories, 0 files
> 
> I think it's didn't unregistered.. I may forget. I need to apply one of
> patch from Matthew ?

You need to specify i915.take_over_backlight=1 in kernel cmdline, that
module option is set to false by default for now.

Thanks for the test.

-Aaron

> 
> Some strings from logs:
> DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013
> thinkpad_acpi: Standard ACPI backlight interface available, not loading
> native one
> 

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Igor Gnatenko]

On Tue, 2013-09-10 at 13:16 +0800, Aaron Lu wrote:
> On 09/10/2013 01:13 PM, Igor Gnatenko wrote:
> > On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote:
> >> On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
> >>> On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
> >>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> >>>> index f466980..75fba17 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_dma.c
> >>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
> >>>> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >>>>  	if (INTEL_INFO(dev)->num_pipes) {
> >>>>  		/* Must be done after probing outputs */
> >>>>  		intel_opregion_init(dev);
> >>>> -		acpi_video_register();
> >>>> +		__acpi_video_register(i915_take_over_backlight);
> >>>>  	}
> >>>>  
> >>>>  	if (IS_GEN5(dev))
> >>>
> >>> I can't compile:
> >>>
> >>>
> >>> DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load':
> >>> DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit
> >>> declaration of function
> >>> '__acpi_video_register' [-Werror=implicit-function-declaration]
> >>> DEBUG:    __acpi_video_register(i915_take_over_backlight);
> >>> DEBUG:    ^
> >>> DEBUG: cc1: some warnings being treated as errors
> >>> DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1
> >>> DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2
> >>> DEBUG: make[2]: *** [drivers/gpu/drm] Error 2
> >>> DEBUG: make[1]: *** [drivers/gpu] Error 2
> >>> DEBUG: make: *** [drivers] Error 2
> >>>
> >>
> >> The two patches are based on top of Rafael's linux-next tree. I just
> >> tried it again, no compile problem for me. I also tried on today Linus'
> >> master tree, as there are some updates from i915, two conflicts exist.
> >> I've just resolved them and will update it in next revision.
> >> If you want to try it now, please use:
> >> https://github.com/aaronlu/linux acpi_video_rework
> >>
> >> Thanks,
> >> Aaron
> > 
> > Thanks. this patch fixes my problems w/ compilation. I've tested this
> > two patches and after apply I have:
> > $ tree /sys/class/backlight/
> > /sys/class/backlight/
> > |-- acpi_video0
> > -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0
> > `-- intel_backlight
> > -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
> > 
> > 2 directories, 0 files
> > 
> > I think it's didn't unregistered.. I may forget. I need to apply one of
> > patch from Matthew ?
> 
> You need to specify i915.take_over_backlight=1 in kernel cmdline, that
> module option is set to false by default for now.
> 
> Thanks for the test.
> 
> -Aaron
> 
> > 
> > Some strings from logs:
> > DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013
> > thinkpad_acpi: Standard ACPI backlight interface available, not loading
> > native one
> > 
> 

Thanks for quick answer. Yes. This option do unregister. Thanks. but for
this patch-set I also need "[PATCH 2/3] ACPI / video: Always call
acpi_video_init_brightness() on init" from Matthew (for notifications in
DE).

-- 
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.0-1.fc20.x86_64

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Aaron Lu]

On 09/10/2013 01:22 PM, Igor Gnatenko wrote:
> On Tue, 2013-09-10 at 13:16 +0800, Aaron Lu wrote:
>> On 09/10/2013 01:13 PM, Igor Gnatenko wrote:
>>> On Tue, 2013-09-10 at 11:27 +0800, Aaron Lu wrote:
>>>> On 09/09/2013 07:44 PM, Igor Gnatenko wrote:
>>>>> On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>>>>>> index f466980..75fba17 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>>>>> @@ -1650,7 +1650,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>>>>>  	if (INTEL_INFO(dev)->num_pipes) {
>>>>>>  		/* Must be done after probing outputs */
>>>>>>  		intel_opregion_init(dev);
>>>>>> -		acpi_video_register();
>>>>>> +		__acpi_video_register(i915_take_over_backlight);
>>>>>>  	}
>>>>>>  
>>>>>>  	if (IS_GEN5(dev))
>>>>>
>>>>> I can't compile:
>>>>>
>>>>>
>>>>> DEBUG: drivers/gpu/drm/i915/i915_dma.c: In function 'i915_driver_load':
>>>>> DEBUG: drivers/gpu/drm/i915/i915_dma.c:1661:3: error: implicit
>>>>> declaration of function
>>>>> '__acpi_video_register' [-Werror=implicit-function-declaration]
>>>>> DEBUG:    __acpi_video_register(i915_take_over_backlight);
>>>>> DEBUG:    ^
>>>>> DEBUG: cc1: some warnings being treated as errors
>>>>> DEBUG: make[4]: *** [drivers/gpu/drm/i915/i915_dma.o] Error 1
>>>>> DEBUG: make[3]: *** [drivers/gpu/drm/i915] Error 2
>>>>> DEBUG: make[2]: *** [drivers/gpu/drm] Error 2
>>>>> DEBUG: make[1]: *** [drivers/gpu] Error 2
>>>>> DEBUG: make: *** [drivers] Error 2
>>>>>
>>>>
>>>> The two patches are based on top of Rafael's linux-next tree. I just
>>>> tried it again, no compile problem for me. I also tried on today Linus'
>>>> master tree, as there are some updates from i915, two conflicts exist.
>>>> I've just resolved them and will update it in next revision.
>>>> If you want to try it now, please use:
>>>> https://github.com/aaronlu/linux acpi_video_rework
>>>>
>>>> Thanks,
>>>> Aaron
>>>
>>> Thanks. this patch fixes my problems w/ compilation. I've tested this
>>> two patches and after apply I have:
>>> $ tree /sys/class/backlight/
>>> /sys/class/backlight/
>>> |-- acpi_video0
>>> -> ../../devices/pci0000:00/0000:00:02.0/backlight/acpi_video0
>>> `-- intel_backlight
>>> -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-LVDS-1/intel_backlight
>>>
>>> 2 directories, 0 files
>>>
>>> I think it's didn't unregistered.. I may forget. I need to apply one of
>>> patch from Matthew ?
>>
>> You need to specify i915.take_over_backlight=1 in kernel cmdline, that
>> module option is set to false by default for now.
>>
>> Thanks for the test.
>>
>> -Aaron
>>
>>>
>>> Some strings from logs:
>>> DMI: LENOVO 23205NG/23205NG, BIOS G2ET92WW (2.52 ) 02/22/2013
>>> thinkpad_acpi: Standard ACPI backlight interface available, not loading
>>> native one
>>>
>>
> 
> Thanks for quick answer. Yes. This option do unregister. Thanks. but for
> this patch-set I also need "[PATCH 2/3] ACPI / video: Always call
> acpi_video_init_brightness() on init" from Matthew (for notifications in
> DE).

That patch is reverted as it cause problem for other system:
https://bugs.freedesktop.org/show_bug.cgi?id=68355

OTOH, the thinkpad-acpi module already has a call to _BCL except that
the tpacpi_acpi_handle_locate failed to locate video controller's handle:
https://bugzilla.kernel.org/show_bug.cgi?id=51231#c121
I'll see if I can figure out why.

Thanks,
Aaron

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Igor Gnatenko]

On Mon, 2013-09-09 at 16:42 +0800, Aaron Lu wrote:
> According to Matthew Garrett, "Windows 8 leaves backlight control up
> to individual graphics drivers rather than making ACPI calls itself.
> There's plenty of evidence to suggest that the Intel driver for
> Windows [8] doesn't use the ACPI interface, including the fact that
> it's broken on a bunch of machines when the OS claims to support
> Windows 8.  The simplest thing to do appears to be to disable the
> ACPI backlight interface on these systems".
> 
> There's a problem with that approach, however, because simply
> avoiding to register the ACPI backlight interface if the firmware
> calls _OSI for Windows 8 may not work in the following situations:
>  (1) The ACPI backlight interface actually works on the given system
>      and the i915 driver is not loaded (e.g. another graphics driver
>      is used).
>  (2) The ACPI backlight interface doesn't work on the given system,
>      but there is a vendor platform driver that will register its
>      own, equally broken, backlight interface if not prevented from
>      doing so by the ACPI subsystem.
> Therefore we need to allow the ACPI backlight interface to be
> registered until the i915 driver is loaded which then will unregister
> it if the firmware has called _OSI for Windows 8 (or will register
> the ACPI video driver without backlight support if not already
> present).
> 
> For this reason, introduce an alternative function for registering
> ACPI video, __acpi_video_register(bool), that if ture is passed,
> will check whether or not the ACPI video driver has already been
> registered and whether or not the backlight Windows 8 quirk has to
> be applied. If the quirk has to be applied, it will block the ACPI
> backlight support and either unregister the backlight interface if
> the ACPI video driver has already been registered, or register the
> ACPI video driver without the backlight interface otherwise.  Make
> the i915 driver use __acpi_video_register() instead of
> acpi_video_register() in i915_driver_load(), and the param passed
> there is controlled by the i915 module level parameter
> i915_take_over_backlight, which is set to false by default.
> 
> This change is evolved from earlier patches of Matthew Garrett,
> Chun-Yi Lee and Seth Forshee and is heavily based on two patches
> from Rafael:
> https://lkml.org/lkml/2013/7/17/720
> https://lkml.org/lkml/2013/7/24/806
> 
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
Tested-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
> ---
>  drivers/acpi/internal.h         |  2 ++
>  drivers/acpi/video.c            | 24 ++++++++++++++++--------
>  drivers/acpi/video_detect.c     | 15 ++++++++++++++-
>  drivers/gpu/drm/i915/i915_dma.c |  2 +-
>  drivers/gpu/drm/i915/i915_drv.c |  5 +++++
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  include/acpi/video.h            |  9 +++++++--
>  include/linux/acpi.h            |  1 +
>  8 files changed, 47 insertions(+), 12 deletions(-)

-- 
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.0-3.fc20.x86_64

-- 
Igor Gnatenko
Fedora release 20 (Heisenbug)
Linux 3.11.0-1.fc20.x86_64