Check for ACPI backlight support otherwise use vendor ACPI drivers - version 4

Check for ACPI backlight support otherwise use vendor ACPI drivers - version 4

[Thomas Renninger]

Modifications to version 3:
  - Stop checking for backlight capabilities on devices which cannot
    be a video device (thanks to Peter <nokos@gmx.net>
  - Do not stop checking on the first video device which has no backlight
    support (many thanks to Peter <nokos@gmx.net>
  - Removed a half wrong comment, found by Zhang Rui.

Please queue for 2.6.28.
Tell me if parts do not patch and I can repost them against
whatever is needed.

Ideally they should go in together with Matthew's/Hong's IGD
patches (which should come mainline through the dri tree), these
patches should need mine.

[PATCH 01/10] ACPI: video: Ignore devices that aren't present in hardware

[Thomas Renninger]

This is a reimplemention of commit
0119509c4fbc9adcef1472817fda295334612976
from Matthew Garrett <mjg59@srcf.ucam.org>

This patch got removed because of a regression: ThinkPads with a
Intel graphics card and an Integrated Graphics Device BIOS implementation
stopped working.
In fact, they only worked because the ACPI device of the discrete, the
wrong one, got used (via int10). So ACPI functions were poking on the wrong
hardware used which is a sever bug.
The next patch provides support for above ThinkPads to be able to
switch brightness via the legacy thinkpad_acpi driver and automatically
detect when to use it.

Original commit message from Matthew Garrett:
    Vendors often ship machines with a choice of integrated or discrete
    graphics, and use the same DSDT for both. As a result, the ACPI video
    module will locate devices that may not exist on this specific platform.
    Attempt to determine whether the device exists or not, and abort the
    device creation if it doesn't.

http://bugzilla.kernel.org/show_bug.cgi?id=9614

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/acpi/glue.c     |   40 ++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/video.c    |    7 ++++++-
 include/acpi/acpi_bus.h |    2 ++
 3 files changed, 48 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 2f173e8..edb5590 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -140,6 +140,46 @@ struct device *acpi_get_physical_device(acpi_handle handle)
 
 EXPORT_SYMBOL(acpi_get_physical_device);
 
+/* ToDo: When a PCI bridge is found, return the PCI device behind the bridge
+ *       This should work in general, but did not on a Lenovo T61 for the
+ *	 graphics card. But this must be fixed when the PCI device is
+ *       bound and the kernel device struct is attached to the acpi device
+ * Note: A success call will increase reference count by one
+ *       Do call put_device(dev) on the returned device then
+ */
+struct device *acpi_get_physical_pci_device(acpi_handle handle)
+{
+	struct device *dev;
+	long device_id;
+	acpi_status status;
+
+	status =
+		acpi_evaluate_integer(handle, "_ADR", NULL, &device_id);
+
+	if (ACPI_FAILURE(status))
+		return NULL;
+
+	/* We need to attempt to determine whether the _ADR refers to a
+	   PCI device or not. There's no terribly good way to do this,
+	   so the best we can hope for is to assume that there'll never
+	   be a device in the host bridge */
+	if (device_id >= 0x10000) {
+		/* It looks like a PCI device. Does it exist? */
+		dev = acpi_get_physical_device(handle);
+	} else {
+		/* It doesn't look like a PCI device. Does its parent
+		   exist? */
+		acpi_handle phandle;
+		if (acpi_get_parent(handle, &phandle))
+			return NULL;
+		dev = acpi_get_physical_device(phandle);
+	}
+	if (!dev)
+		return NULL;
+	return dev;
+}
+EXPORT_SYMBOL(acpi_get_physical_pci_device);
+
 static int acpi_bind_one(struct device *dev, acpi_handle handle)
 {
 	struct acpi_device *acpi_dev;
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index e32b6c1..649888a 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -843,11 +843,16 @@ static void acpi_video_bus_find_cap(struct acpi_video_bus *video)
 static int acpi_video_bus_check(struct acpi_video_bus *video)
 {
 	acpi_status status = -ENOENT;
-
+	struct device *dev;
 
 	if (!video)
 		return -EINVAL;
 
+	dev = acpi_get_physical_pci_device(video->device->handle);
+	if (!dev)
+		return -ENODEV;
+	put_device(dev);
+
 	/* Since there is no HID, CID and so on for VGA driver, we have
 	 * to check well known required nodes.
 	 */
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index a5ac0bc..234733c 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -373,6 +373,8 @@ struct acpi_bus_type {
 int register_acpi_bus_type(struct acpi_bus_type *);
 int unregister_acpi_bus_type(struct acpi_bus_type *);
 struct device *acpi_get_physical_device(acpi_handle);
+struct device *acpi_get_physical_pci_device(acpi_handle);
+
 /* helper */
 acpi_handle acpi_get_child(acpi_handle, acpi_integer);
 acpi_handle acpi_get_pci_rootbridge_handle(unsigned int, unsigned int);
-- 
1.5.4.5

[PATCH 02/10] Check for ACPI backlight support otherwise use vendor ACPI drivers

[Thomas Renninger]

If an ACPI graphics device supports backlight brightness functions (cmp. with
latest ACPI spec Appendix B), let the ACPI video driver control backlight and
switch backlight control off in vendor specific ACPI drivers (asus_acpi,
thinkpad_acpi, eeepc, fujitsu_laptop, msi_laptop, sony_laptop, acer-wmi).

Currently it is possible to load above drivers and let both poke on the
brightness HW registers, the video and vendor specific ACPI drivers -> bad.

This patch provides the basic support to check for BIOS capabilities before
driver loading time. Driver specific modifications are in separate follow up
patches.

acpi_backlight=vendor/video
      boot params forces video.ko or vendor specific drivers to keep its
      fingers off backlight control even it would find needed functions.
      The corresponding vendor specific driver be used then.

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 Documentation/kernel-parameters.txt |   13 ++
 drivers/acpi/Makefile               |    5 +
 drivers/acpi/scan.c                 |   32 +----
 drivers/acpi/video.c                |   28 ++--
 drivers/acpi/video_detect.c         |  268 +++++++++++++++++++++++++++++++++++
 include/linux/acpi.h                |   44 ++++++
 6 files changed, 347 insertions(+), 43 deletions(-)
 create mode 100644 drivers/acpi/video_detect.c

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 09ad745..0fea7c2 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -194,6 +194,19 @@ and is between 256 and 4096 characters. It is defined in the file
 			that require a timer override, but don't have
 			HPET
 
+	acpi_backlight=	[HW,ACPI]
+			acpi_backlight=vendor
+			acpi_backlight=video
+			If set to vendor, it enforces the use of a
+			vendor specific ACPI driver for backlight switching
+			(e.g. thinkpad_acpi, sony_acpi, etc.) instead
+			of the video.ko driver.
+
+	acpi_display_output=	[HW,ACPI]
+			acpi_display_output=vendor
+			acpi_display_output=video
+			See above.
+
 	acpi.debug_layer=	[HW,ACPI]
 			Format: <int>
 			Each bit of the <int> indicates an ACPI debug layer,
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 52a4cd4..6b7ad0f 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -46,7 +46,12 @@ obj-$(CONFIG_ACPI_BUTTON)	+= button.o
 obj-$(CONFIG_ACPI_FAN)		+= fan.o
 obj-$(CONFIG_ACPI_DOCK)		+= dock.o
 obj-$(CONFIG_ACPI_BAY)		+= bay.o
+
 obj-$(CONFIG_ACPI_VIDEO)	+= video.o
+ifdef CONFIG_ACPI_VIDEO
+obj-y				+= video_detect.o
+endif
+
 obj-y				+= pci_root.o pci_link.o pci_irq.o pci_bind.o
 obj-$(CONFIG_ACPI_PCI_SLOT)	+= pci_slot.o
 obj-$(CONFIG_ACPI_POWER)	+= power.o
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index f3132aa..2dcb953 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -901,36 +901,6 @@ static void acpi_device_get_busid(struct acpi_device *device,
 	}
 }
 
-static int
-acpi_video_bus_match(struct acpi_device *device)
-{
-	acpi_handle h_dummy;
-
-	if (!device)
-		return -EINVAL;
-
-	/* Since there is no HID, CID for ACPI Video drivers, we have
-	 * to check well known required nodes for each feature we support.
-	 */
-
-	/* Does this device able to support video switching ? */
-	if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_DOD", &h_dummy)) &&
-	    ACPI_SUCCESS(acpi_get_handle(device->handle, "_DOS", &h_dummy)))
-		return 0;
-
-	/* Does this device able to retrieve a video ROM ? */
-	if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_ROM", &h_dummy)))
-		return 0;
-
-	/* Does this device able to configure which video head to be POSTed ? */
-	if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_VPO", &h_dummy)) &&
-	    ACPI_SUCCESS(acpi_get_handle(device->handle, "_GPD", &h_dummy)) &&
-	    ACPI_SUCCESS(acpi_get_handle(device->handle, "_SPD", &h_dummy)))
-		return 0;
-
-	return -ENODEV;
-}
-
 /*
  * acpi_bay_match - see if a device is an ejectable driver bay
  *
@@ -1013,7 +983,7 @@ static void acpi_device_set_id(struct acpi_device *device,
 		   will get autoloaded and the device might still match
 		   against another driver.
 		*/
-		if (ACPI_SUCCESS(acpi_video_bus_match(device)))
+		if (acpi_is_video_device(device))
 			cid_add = ACPI_VIDEO_HID;
 		else if (ACPI_SUCCESS(acpi_bay_match(device)))
 			cid_add = ACPI_BAY_HID;
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 649888a..7abd326 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -739,7 +739,8 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 		device->cap._DSS = 1;
 	}
 
-	max_level = acpi_video_init_brightness(device);
+	if (acpi_video_backlight_support())
+		max_level = acpi_video_init_brightness(device);
 
 	if (device->cap._BCL && device->cap._BCM && max_level > 0) {
 		int result;
@@ -786,18 +787,21 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 			printk(KERN_ERR PREFIX "Create sysfs link\n");
 
 	}
-	if (device->cap._DCS && device->cap._DSS){
-		static int count = 0;
-		char *name;
-		name = kzalloc(MAX_NAME_LEN, GFP_KERNEL);
-		if (!name)
-			return;
-		sprintf(name, "acpi_video%d", count++);
-		device->output_dev = video_output_register(name,
-				NULL, device, &acpi_output_properties);
-		kfree(name);
+
+	if (acpi_video_display_switch_support()) {
+
+		if (device->cap._DCS && device->cap._DSS) {
+			static int count;
+			char *name;
+			name = kzalloc(MAX_NAME_LEN, GFP_KERNEL);
+			if (!name)
+				return;
+			sprintf(name, "acpi_video%d", count++);
+			device->output_dev = video_output_register(name,
+					NULL, device, &acpi_output_properties);
+			kfree(name);
+		}
 	}
-	return;
 }
 
 /*
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
new file mode 100644
index 0000000..85c3d2c
--- /dev/null
+++ b/drivers/acpi/video_detect.c
@@ -0,0 +1,268 @@
+/*
+ *  Copyright (C) 2008       SuSE Linux Products GmbH
+ *                           Thomas Renninger <trenn@suse.de>
+ *
+ *  May be copied or modified under the terms of the GNU General Public License
+ *
+ * video_detect.c:
+ * Provides acpi_is_video_device() for early scanning of ACPI devices in scan.c
+ * There a Linux specific (Spec does not provide a HID for video devices) is
+ * assinged
+ *
+ * After PCI devices are glued with ACPI devices
+ * acpi_get_physical_pci_device() can be called to identify ACPI graphics
+ * devices for which a real graphics card is plugged in
+ *
+ * Now acpi_video_get_capabilities() can be called to check which
+ * capabilities the graphics cards plugged in support. The check for general
+ * video capabilities will be triggered by the first caller of
+ * acpi_video_get_capabilities(NULL); which will happen when the first
+ * backlight (or display output) switching supporting driver calls:
+ * acpi_video_backlight_support();
+ *
+ * Depending on whether ACPI graphics extensions (cmp. ACPI spec Appendix B)
+ * are available, video.ko should be used to handle the device.
+ *
+ * Otherwise vendor specific drivers like thinkpad_acpi, asus_acpi,
+ * sony_acpi,... can take care about backlight brightness and display output
+ * switching.
+ *
+ * If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a module (m)
+ * this file will not be compiled, acpi_video_get_capabilities() and
+ * acpi_video_backlight_support() will always return 0 and vendor specific
+ * drivers always can handle backlight.
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+
+ACPI_MODULE_NAME("video");
+#define ACPI_VIDEO_COMPONENT		0x08000000
+#define _COMPONENT		ACPI_VIDEO_COMPONENT
+
+static long acpi_video_support;
+static bool acpi_video_caps_checked;
+
+static acpi_status
+acpi_backlight_cap_match(acpi_handle handle, u32 level, void *context,
+			  void **retyurn_value)
+{
+	long *cap = context;
+	acpi_handle h_dummy;
+
+	if (ACPI_SUCCESS(acpi_get_handle(handle, "_BCM", &h_dummy)) &&
+	    ACPI_SUCCESS(acpi_get_handle(handle, "_BCL", &h_dummy))) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found generic backlight "
+				  "support\n"));
+		*cap |= ACPI_VIDEO_BACKLIGHT;
+		/* We have backlight support, no need to scan further */
+		return AE_CTRL_TERMINATE;
+	}
+	return 0;
+}
+
+/* Returns true if the device is a video device which can be handled by
+ * video.ko.
+ * The device will get a Linux specific CID added in scan.c to
+ * identify the device as an ACPI graphics device
+ * Be aware that the graphics device may not be physically present
+ * Use acpi_video_get_capabilities() to detect general ACPI video
+ * capabilities of present cards
+ */
+long acpi_is_video_device(struct acpi_device *device)
+{
+	acpi_handle h_dummy;
+	long video_caps = 0;
+
+	if (!device)
+		return 0;
+
+	/* Does this device able to support video switching ? */
+	if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_DOD", &h_dummy)) &&
+	    ACPI_SUCCESS(acpi_get_handle(device->handle, "_DOS", &h_dummy)))
+		video_caps |= ACPI_VIDEO_OUTPUT_SWITCHING;
+
+	/* Does this device able to retrieve a video ROM ? */
+	if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_ROM", &h_dummy)))
+		video_caps |= ACPI_VIDEO_ROM_AVAILABLE;
+
+	/* Does this device able to configure which video head to be POSTed ? */
+	if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_VPO", &h_dummy)) &&
+	    ACPI_SUCCESS(acpi_get_handle(device->handle, "_GPD", &h_dummy)) &&
+	    ACPI_SUCCESS(acpi_get_handle(device->handle, "_SPD", &h_dummy)))
+		video_caps |= ACPI_VIDEO_DEVICE_POSTING;
+
+	/* Only check for backlight functionality if one of the above hit. */
+	if (video_caps)
+		acpi_walk_namespace(ACPI_TYPE_DEVICE, device->handle,
+				    ACPI_UINT32_MAX, acpi_backlight_cap_match,
+				    &video_caps, NULL);
+
+	return video_caps;
+}
+EXPORT_SYMBOL(acpi_is_video_device);
+
+static acpi_status
+find_video(acpi_handle handle, u32 lvl, void *context, void **rv)
+{
+	long *cap = context;
+	struct device *dev;
+	struct acpi_device *acpi_dev;
+
+	const struct acpi_device_id video_ids[] = {
+		{ACPI_VIDEO_HID, 0},
+		{"", 0},
+	};
+	if (acpi_bus_get_device(handle, &acpi_dev))
+		return AE_OK;
+
+	if (!acpi_match_device_ids(acpi_dev, video_ids)) {
+		dev = acpi_get_physical_pci_device(handle);
+		if (!dev)
+			return AE_OK;
+		put_device(dev);
+		*cap |= acpi_is_video_device(acpi_dev);
+	}
+	return AE_OK;
+}
+
+/*
+ * Returns the video capabilities of a specific ACPI graphics device
+ *
+ * if NULL is passed as argument all ACPI devices are enumerated and
+ * all graphics capabilities of physically present devices are
+ * summerized and returned. This is cached and done only once.
+ */
+long acpi_video_get_capabilities(acpi_handle graphics_handle)
+{
+	long caps = 0;
+	struct acpi_device *tmp_dev;
+	acpi_status status;
+
+	if (acpi_video_caps_checked && graphics_handle == NULL)
+		return acpi_video_support;
+
+	if (!graphics_handle) {
+		/* Only do the global walk through all graphics devices once */
+		acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+				    ACPI_UINT32_MAX, find_video,
+				    &caps, NULL);
+		/* There might be boot param flags set already... */
+		acpi_video_support |= caps;
+		acpi_video_caps_checked = 1;
+		/* Add blacklists here. Be careful to use the right *DMI* bits
+		 * to still be able to override logic via boot params, e.g.:
+		 *
+		 *   if (dmi_name_in_vendors("XY")) {
+		 *	acpi_video_support |=
+		 *		ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR;
+		 *	acpi_video_support |=
+		 *		ACPI_VIDEO_BACKLIGHT_DMI_VENDOR;
+		 *}
+		 */
+	} else {
+		status = acpi_bus_get_device(graphics_handle, &tmp_dev);
+		if (ACPI_FAILURE(status)) {
+			ACPI_EXCEPTION((AE_INFO, status, "Invalid device"));
+			return 0;
+		}
+		acpi_walk_namespace(ACPI_TYPE_DEVICE, graphics_handle,
+				    ACPI_UINT32_MAX, find_video,
+				    &caps, NULL);
+	}
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "We have 0x%lX video support %s %s\n",
+			  graphics_handle ? caps : acpi_video_support,
+			  graphics_handle ? "on device " : "in general",
+			  graphics_handle ? acpi_device_bid(tmp_dev) : ""));
+	return caps;
+}
+EXPORT_SYMBOL(acpi_video_get_capabilities);
+
+/* Returns true if video.ko can do backlight switching */
+int acpi_video_backlight_support(void)
+{
+	/*
+	 * We must check whether the ACPI graphics device is physically plugged
+	 * in. Therefore this must be called after binding PCI and ACPI devices
+	 */
+	if (!acpi_video_caps_checked)
+		acpi_video_get_capabilities(NULL);
+
+	/* First check for boot param -> highest prio */
+	if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR)
+		return 0;
+	else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO)
+		return 1;
+
+	/* Then check for DMI blacklist -> second highest prio */
+	if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VENDOR)
+		return 0;
+	else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VIDEO)
+		return 1;
+
+	/* Then go the default way */
+	return acpi_video_support & ACPI_VIDEO_BACKLIGHT;
+}
+EXPORT_SYMBOL(acpi_video_backlight_support);
+
+/*
+ * Returns true if video.ko can do display output switching.
+ * This does not work well/at all with binary graphics drivers
+ * which disable system io ranges and do it on their own.
+ */
+int acpi_video_display_switch_support(void)
+{
+	if (!acpi_video_caps_checked)
+		acpi_video_get_capabilities(NULL);
+
+	if (acpi_video_support & ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VENDOR)
+		return 0;
+	else if (acpi_video_support & ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VIDEO)
+		return 1;
+
+	if (acpi_video_support & ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR)
+		return 0;
+	else if (acpi_video_support & ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO)
+		return 1;
+
+	return acpi_video_support & ACPI_VIDEO_OUTPUT_SWITCHING;
+}
+EXPORT_SYMBOL(acpi_video_display_switch_support);
+
+/* 
+ * Use acpi_display_output=vendor/video or acpi_backlight=vendor/video
+ * To force that backlight or display output switching is processed by vendor
+ * specific acpi drivers or video.ko driver.
+ */
+int __init acpi_backlight(char *str)
+{
+	if (str == NULL || *str == '\0')
+		return 1;
+	else {
+		if (!strcmp("vendor", str))
+			acpi_video_support |=
+				ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
+		if (!strcmp("video", str))
+			acpi_video_support |=
+				ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VIDEO;
+	}
+	return 1;
+}
+__setup("acpi_backlight=", acpi_backlight);
+
+int __init acpi_display_output(char *str)
+{
+	if (str == NULL || *str == '\0')
+		return 1;
+	else {
+		if (!strcmp("vendor", str))
+			acpi_video_support |=
+				ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VENDOR;
+		if (!strcmp("video", str))
+			acpi_video_support |=
+				ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VIDEO;
+	}
+	return 1;
+}
+__setup("acpi_display_output=", acpi_display_output);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a171776..eb51726 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -202,6 +202,50 @@ extern bool wmi_has_guid(const char *guid);
 
 #endif	/* CONFIG_ACPI_WMI */
 
+#define ACPI_VIDEO_OUTPUT_SWITCHING			0x0001
+#define ACPI_VIDEO_DEVICE_POSTING			0x0002
+#define ACPI_VIDEO_ROM_AVAILABLE			0x0004
+#define ACPI_VIDEO_BACKLIGHT				0x0008
+#define ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR		0x0010
+#define ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO		0x0020
+#define ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VENDOR	0x0040
+#define ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VIDEO		0x0080
+#define ACPI_VIDEO_BACKLIGHT_DMI_VENDOR			0x0100
+#define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO			0x0200
+#define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR		0x0400
+#define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO		0x0800
+
+#if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
+
+extern long acpi_video_get_capabilities(acpi_handle graphics_dev_handle);
+extern long acpi_is_video_device(struct acpi_device *device);
+extern int acpi_video_backlight_support(void);
+extern int acpi_video_display_switch_support(void);
+
+#else
+
+static inline long acpi_video_get_capabilities(acpi_handle graphics_dev_handle)
+{
+	return 0;
+}
+
+static inline long acpi_is_video_device(struct acpi_device *device)
+{
+	return 0;
+}
+
+static inline int acpi_video_backlight_support(void)
+{
+	return 0;
+}
+
+static inline int acpi_video_display_switch_support(void)
+{
+	return 0;
+}
+
+#endif /* defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) */
+
 extern int acpi_blacklisted(void);
 #ifdef CONFIG_DMI
 extern void acpi_dmi_osi_linux(int enable, const struct dmi_system_id *d);
-- 
1.5.4.5

Re: [PATCH 02/10] Check for ACPI backlight support otherwise use vendor ACPI drivers

[Zhang Rui]

On Fri, 2008-08-01 at 23:37 +0800, Thomas Renninger wrote:
> If an ACPI graphics device supports backlight brightness functions
> (cmp. with
> latest ACPI spec Appendix B), let the ACPI video driver control
> backlight and
> switch backlight control off in vendor specific ACPI drivers
> (asus_acpi,
> thinkpad_acpi, eeepc, fujitsu_laptop, msi_laptop, sony_laptop,
> acer-wmi).
> 
> Currently it is possible to load above drivers and let both poke on
> the
> brightness HW registers, the video and vendor specific ACPI drivers ->
> bad.
> 
> This patch provides the basic support to check for BIOS capabilities
> before
> driver loading time. Driver specific modifications are in separate
> follow up
> patches.
> 
> acpi_backlight=vendor/video
>       boot params forces video.ko or vendor specific drivers to keep
> its
>       fingers off backlight control even it would find needed
> functions.
>       The corresponding vendor specific driver be used then.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>
Acked-by: Zhang Rui <rui.zhang@intel.com>

> ---
>  Documentation/kernel-parameters.txt |   13 ++
>  drivers/acpi/Makefile               |    5 +
>  drivers/acpi/scan.c                 |   32 +----
>  drivers/acpi/video.c                |   28 ++--
>  drivers/acpi/video_detect.c         |  268
> +++++++++++++++++++++++++++++++++++
>  include/linux/acpi.h                |   44 ++++++
>  6 files changed, 347 insertions(+), 43 deletions(-)
>  create mode 100644 drivers/acpi/video_detect.c
> 
> diff --git a/Documentation/kernel-parameters.txt
> b/Documentation/kernel-parameters.txt
> index 09ad745..0fea7c2 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -194,6 +194,19 @@ and is between 256 and 4096 characters. It is
> defined in the file
>                         that require a timer override, but don't have
>                         HPET
> 
> +       acpi_backlight= [HW,ACPI]
> +                       acpi_backlight=vendor
> +                       acpi_backlight=video
> +                       If set to vendor, it enforces the use of a
> +                       vendor specific ACPI driver for backlight
> switching
> +                       (e.g. thinkpad_acpi, sony_acpi, etc.) instead
> +                       of the video.ko driver.
> +
> +       acpi_display_output=    [HW,ACPI]
> +                       acpi_display_output=vendor
> +                       acpi_display_output=video
> +                       See above.
> +
>         acpi.debug_layer=       [HW,ACPI]
>                         Format: <int>
>                         Each bit of the <int> indicates an ACPI debug
> layer,
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 52a4cd4..6b7ad0f 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -46,7 +46,12 @@ obj-$(CONFIG_ACPI_BUTTON)    += button.o
>  obj-$(CONFIG_ACPI_FAN)         += fan.o
>  obj-$(CONFIG_ACPI_DOCK)                += dock.o
>  obj-$(CONFIG_ACPI_BAY)         += bay.o
> +
>  obj-$(CONFIG_ACPI_VIDEO)       += video.o
> +ifdef CONFIG_ACPI_VIDEO
> +obj-y                          += video_detect.o
> +endif
> +
>  obj-y                          += pci_root.o pci_link.o pci_irq.o
> pci_bind.o
>  obj-$(CONFIG_ACPI_PCI_SLOT)    += pci_slot.o
>  obj-$(CONFIG_ACPI_POWER)       += power.o
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index f3132aa..2dcb953 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -901,36 +901,6 @@ static void acpi_device_get_busid(struct
> acpi_device *device,
>         }
>  }
> 
> -static int
> -acpi_video_bus_match(struct acpi_device *device)
> -{
> -       acpi_handle h_dummy;
> -
> -       if (!device)
> -               return -EINVAL;
> -
> -       /* Since there is no HID, CID for ACPI Video drivers, we have
> -        * to check well known required nodes for each feature we
> support.
> -        */
> -
> -       /* Does this device able to support video switching ? */
> -       if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_DOD",
> &h_dummy)) &&
> -           ACPI_SUCCESS(acpi_get_handle(device->handle, "_DOS",
> &h_dummy)))
> -               return 0;
> -
> -       /* Does this device able to retrieve a video ROM ? */
> -       if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_ROM",
> &h_dummy)))
> -               return 0;
> -
> -       /* Does this device able to configure which video head to be
> POSTed ? */
> -       if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_VPO",
> &h_dummy)) &&
> -           ACPI_SUCCESS(acpi_get_handle(device->handle, "_GPD",
> &h_dummy)) &&
> -           ACPI_SUCCESS(acpi_get_handle(device->handle, "_SPD",
> &h_dummy)))
> -               return 0;
> -
> -       return -ENODEV;
> -}
> -
>  /*
>   * acpi_bay_match - see if a device is an ejectable driver bay
>   *
> @@ -1013,7 +983,7 @@ static void acpi_device_set_id(struct acpi_device
> *device,
>                    will get autoloaded and the device might still
> match
>                    against another driver.
>                 */
> -               if (ACPI_SUCCESS(acpi_video_bus_match(device)))
> +               if (acpi_is_video_device(device))
>                         cid_add = ACPI_VIDEO_HID;
>                 else if (ACPI_SUCCESS(acpi_bay_match(device)))
>                         cid_add = ACPI_BAY_HID;
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 649888a..7abd326 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -739,7 +739,8 @@ static void acpi_video_device_find_cap(struct
> acpi_video_device *device)
>                 device->cap._DSS = 1;
>         }
> 
> -       max_level = acpi_video_init_brightness(device);
> +       if (acpi_video_backlight_support())
> +               max_level = acpi_video_init_brightness(device);
> 
>         if (device->cap._BCL && device->cap._BCM && max_level > 0) {
>                 int result;
> @@ -786,18 +787,21 @@ static void acpi_video_device_find_cap(struct
> acpi_video_device *device)
>                         printk(KERN_ERR PREFIX "Create sysfs link\n");
> 
>         }
> -       if (device->cap._DCS && device->cap._DSS){
> -               static int count = 0;
> -               char *name;
> -               name = kzalloc(MAX_NAME_LEN, GFP_KERNEL);
> -               if (!name)
> -                       return;
> -               sprintf(name, "acpi_video%d", count++);
> -               device->output_dev = video_output_register(name,
> -                               NULL, device,
> &acpi_output_properties);
> -               kfree(name);
> +
> +       if (acpi_video_display_switch_support()) {
> +
> +               if (device->cap._DCS && device->cap._DSS) {
> +                       static int count;
> +                       char *name;
> +                       name = kzalloc(MAX_NAME_LEN, GFP_KERNEL);
> +                       if (!name)
> +                               return;
> +                       sprintf(name, "acpi_video%d", count++);
> +                       device->output_dev =
> video_output_register(name,
> +                                       NULL, device,
> &acpi_output_properties);
> +                       kfree(name);
> +               }
>         }
> -       return;
>  }
> 
>  /*
> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> new file mode 100644
> index 0000000..85c3d2c
> --- /dev/null
> +++ b/drivers/acpi/video_detect.c
> @@ -0,0 +1,268 @@
> +/*
> + *  Copyright (C) 2008       SuSE Linux Products GmbH
> + *                           Thomas Renninger <trenn@suse.de>
> + *
> + *  May be copied or modified under the terms of the GNU General
> Public License
> + *
> + * video_detect.c:
> + * Provides acpi_is_video_device() for early scanning of ACPI devices
> in scan.c
> + * There a Linux specific (Spec does not provide a HID for video
> devices) is
> + * assinged
> + *
> + * After PCI devices are glued with ACPI devices
> + * acpi_get_physical_pci_device() can be called to identify ACPI
> graphics
> + * devices for which a real graphics card is plugged in
> + *
> + * Now acpi_video_get_capabilities() can be called to check which
> + * capabilities the graphics cards plugged in support. The check for
> general
> + * video capabilities will be triggered by the first caller of
> + * acpi_video_get_capabilities(NULL); which will happen when the
> first
> + * backlight (or display output) switching supporting driver calls:
> + * acpi_video_backlight_support();
> + *
> + * Depending on whether ACPI graphics extensions (cmp. ACPI spec
> Appendix B)
> + * are available, video.ko should be used to handle the device.
> + *
> + * Otherwise vendor specific drivers like thinkpad_acpi, asus_acpi,
> + * sony_acpi,... can take care about backlight brightness and display
> output
> + * switching.
> + *
> + * If CONFIG_ACPI_VIDEO is neither set as "compiled in" (y) nor as a
> module (m)
> + * this file will not be compiled, acpi_video_get_capabilities() and
> + * acpi_video_backlight_support() will always return 0 and vendor
> specific
> + * drivers always can handle backlight.
> + *
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +
> +ACPI_MODULE_NAME("video");
> +#define ACPI_VIDEO_COMPONENT           0x08000000
> +#define _COMPONENT             ACPI_VIDEO_COMPONENT
> +
> +static long acpi_video_support;
> +static bool acpi_video_caps_checked;
> +
> +static acpi_status
> +acpi_backlight_cap_match(acpi_handle handle, u32 level, void
> *context,
> +                         void **retyurn_value)
> +{
> +       long *cap = context;
> +       acpi_handle h_dummy;
> +
> +       if (ACPI_SUCCESS(acpi_get_handle(handle, "_BCM", &h_dummy)) &&
> +           ACPI_SUCCESS(acpi_get_handle(handle, "_BCL", &h_dummy))) {
> +               ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found generic
> backlight "
> +                                 "support\n"));
> +               *cap |= ACPI_VIDEO_BACKLIGHT;
> +               /* We have backlight support, no need to scan further
> */
> +               return AE_CTRL_TERMINATE;
> +       }
> +       return 0;
> +}
> +
> +/* Returns true if the device is a video device which can be handled
> by
> + * video.ko.
> + * The device will get a Linux specific CID added in scan.c to
> + * identify the device as an ACPI graphics device
> + * Be aware that the graphics device may not be physically present
> + * Use acpi_video_get_capabilities() to detect general ACPI video
> + * capabilities of present cards
> + */
> +long acpi_is_video_device(struct acpi_device *device)
> +{
> +       acpi_handle h_dummy;
> +       long video_caps = 0;
> +
> +       if (!device)
> +               return 0;
> +
> +       /* Does this device able to support video switching ? */
> +       if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_DOD",
> &h_dummy)) &&
> +           ACPI_SUCCESS(acpi_get_handle(device->handle, "_DOS",
> &h_dummy)))
> +               video_caps |= ACPI_VIDEO_OUTPUT_SWITCHING;
> +
> +       /* Does this device able to retrieve a video ROM ? */
> +       if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_ROM",
> &h_dummy)))
> +               video_caps |= ACPI_VIDEO_ROM_AVAILABLE;
> +
> +       /* Does this device able to configure which video head to be
> POSTed ? */
> +       if (ACPI_SUCCESS(acpi_get_handle(device->handle, "_VPO",
> &h_dummy)) &&
> +           ACPI_SUCCESS(acpi_get_handle(device->handle, "_GPD",
> &h_dummy)) &&
> +           ACPI_SUCCESS(acpi_get_handle(device->handle, "_SPD",
> &h_dummy)))
> +               video_caps |= ACPI_VIDEO_DEVICE_POSTING;
> +
> +       /* Only check for backlight functionality if one of the above
> hit. */
> +       if (video_caps)
> +               acpi_walk_namespace(ACPI_TYPE_DEVICE, device->handle,
> +                                   ACPI_UINT32_MAX,
> acpi_backlight_cap_match,
> +                                   &video_caps, NULL);
> +
> +       return video_caps;
> +}
> +EXPORT_SYMBOL(acpi_is_video_device);
> +
> +static acpi_status
> +find_video(acpi_handle handle, u32 lvl, void *context, void **rv)
> +{
> +       long *cap = context;
> +       struct device *dev;
> +       struct acpi_device *acpi_dev;
> +
> +       const struct acpi_device_id video_ids[] = {
> +               {ACPI_VIDEO_HID, 0},
> +               {"", 0},
> +       };
> +       if (acpi_bus_get_device(handle, &acpi_dev))
> +               return AE_OK;
> +
> +       if (!acpi_match_device_ids(acpi_dev, video_ids)) {
> +               dev = acpi_get_physical_pci_device(handle);
> +               if (!dev)
> +                       return AE_OK;
> +               put_device(dev);
> +               *cap |= acpi_is_video_device(acpi_dev);
> +       }
> +       return AE_OK;
> +}
> +
> +/*
> + * Returns the video capabilities of a specific ACPI graphics device
> + *
> + * if NULL is passed as argument all ACPI devices are enumerated and
> + * all graphics capabilities of physically present devices are
> + * summerized and returned. This is cached and done only once.
> + */
> +long acpi_video_get_capabilities(acpi_handle graphics_handle)
> +{
> +       long caps = 0;
> +       struct acpi_device *tmp_dev;
> +       acpi_status status;
> +
> +       if (acpi_video_caps_checked && graphics_handle == NULL)
> +               return acpi_video_support;
> +
> +       if (!graphics_handle) {
> +               /* Only do the global walk through all graphics
> devices once */
> +               acpi_walk_namespace(ACPI_TYPE_DEVICE,
> ACPI_ROOT_OBJECT,
> +                                   ACPI_UINT32_MAX, find_video,
> +                                   &caps, NULL);
> +               /* There might be boot param flags set already... */
> +               acpi_video_support |= caps;
> +               acpi_video_caps_checked = 1;
> +               /* Add blacklists here. Be careful to use the right
> *DMI* bits
> +                * to still be able to override logic via boot params,
> e.g.:
> +                *
> +                *   if (dmi_name_in_vendors("XY")) {
> +                *      acpi_video_support |=
> +                *
> ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR;
> +                *      acpi_video_support |=
> +                *              ACPI_VIDEO_BACKLIGHT_DMI_VENDOR;
> +                *}
> +                */
> +       } else {
> +               status = acpi_bus_get_device(graphics_handle,
> &tmp_dev);
> +               if (ACPI_FAILURE(status)) {
> +                       ACPI_EXCEPTION((AE_INFO, status, "Invalid
> device"));
> +                       return 0;
> +               }
> +               acpi_walk_namespace(ACPI_TYPE_DEVICE, graphics_handle,
> +                                   ACPI_UINT32_MAX, find_video,
> +                                   &caps, NULL);
> +       }
> +       ACPI_DEBUG_PRINT((ACPI_DB_INFO, "We have 0x%lX video support %
> s %s\n",
> +                         graphics_handle ? caps : acpi_video_support,
> +                         graphics_handle ? "on device " : "in
> general",
> +                         graphics_handle ? acpi_device_bid(tmp_dev) :
> ""));
> +       return caps;
> +}
> +EXPORT_SYMBOL(acpi_video_get_capabilities);
> +
> +/* Returns true if video.ko can do backlight switching */
> +int acpi_video_backlight_support(void)
> +{
> +       /*
> +        * We must check whether the ACPI graphics device is
> physically plugged
> +        * in. Therefore this must be called after binding PCI and
> ACPI devices
> +        */
> +       if (!acpi_video_caps_checked)
> +               acpi_video_get_capabilities(NULL);
> +
> +       /* First check for boot param -> highest prio */
> +       if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR)
> +               return 0;
> +       else if (acpi_video_support &
> ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO)
> +               return 1;
> +
> +       /* Then check for DMI blacklist -> second highest prio */
> +       if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VENDOR)
> +               return 0;
> +       else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VIDEO)
> +               return 1;
> +
> +       /* Then go the default way */
> +       return acpi_video_support & ACPI_VIDEO_BACKLIGHT;
> +}
> +EXPORT_SYMBOL(acpi_video_backlight_support);
> +
> +/*
> + * Returns true if video.ko can do display output switching.
> + * This does not work well/at all with binary graphics drivers
> + * which disable system io ranges and do it on their own.
> + */
> +int acpi_video_display_switch_support(void)
> +{
> +       if (!acpi_video_caps_checked)
> +               acpi_video_get_capabilities(NULL);
> +
> +       if (acpi_video_support &
> ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VENDOR)
> +               return 0;
> +       else if (acpi_video_support &
> ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VIDEO)
> +               return 1;
> +
> +       if (acpi_video_support &
> ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR)
> +               return 0;
> +       else if (acpi_video_support &
> ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO)
> +               return 1;
> +
> +       return acpi_video_support & ACPI_VIDEO_OUTPUT_SWITCHING;
> +}
> +EXPORT_SYMBOL(acpi_video_display_switch_support);
> +
> +/*
> + * Use acpi_display_output=vendor/video or
> acpi_backlight=vendor/video
> + * To force that backlight or display output switching is processed
> by vendor
> + * specific acpi drivers or video.ko driver.
> + */
> +int __init acpi_backlight(char *str)
> +{
> +       if (str == NULL || *str == '\0')
> +               return 1;
> +       else {
> +               if (!strcmp("vendor", str))
> +                       acpi_video_support |=
> +                               ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
> +               if (!strcmp("video", str))
> +                       acpi_video_support |=
> +
> ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VIDEO;
> +       }
> +       return 1;
> +}
> +__setup("acpi_backlight=", acpi_backlight);
> +
> +int __init acpi_display_output(char *str)
> +{
> +       if (str == NULL || *str == '\0')
> +               return 1;
> +       else {
> +               if (!strcmp("vendor", str))
> +                       acpi_video_support |=
> +
> ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VENDOR;
> +               if (!strcmp("video", str))
> +                       acpi_video_support |=
> +
> ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VIDEO;
> +       }
> +       return 1;
> +}
> +__setup("acpi_display_output=", acpi_display_output);
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index a171776..eb51726 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -202,6 +202,50 @@ extern bool wmi_has_guid(const char *guid);
> 
>  #endif /* CONFIG_ACPI_WMI */
> 
> +#define ACPI_VIDEO_OUTPUT_SWITCHING                    0x0001
> +#define ACPI_VIDEO_DEVICE_POSTING                      0x0002
> +#define ACPI_VIDEO_ROM_AVAILABLE                       0x0004
> +#define ACPI_VIDEO_BACKLIGHT                           0x0008
> +#define ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR              0x0010
> +#define ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO               0x0020
> +#define ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VENDOR       0x0040
> +#define ACPI_VIDEO_OUTPUT_SWITCHING_FORCE_VIDEO                0x0080
> +#define ACPI_VIDEO_BACKLIGHT_DMI_VENDOR                        0x0100
> +#define ACPI_VIDEO_BACKLIGHT_DMI_VIDEO                 0x0200
> +#define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VENDOR         0x0400
> +#define ACPI_VIDEO_OUTPUT_SWITCHING_DMI_VIDEO          0x0800
> +
> +#if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
> +
> +extern long acpi_video_get_capabilities(acpi_handle
> graphics_dev_handle);
> +extern long acpi_is_video_device(struct acpi_device *device);
> +extern int acpi_video_backlight_support(void);
> +extern int acpi_video_display_switch_support(void);
> +
> +#else
> +
> +static inline long acpi_video_get_capabilities(acpi_handle
> graphics_dev_handle)
> +{
> +       return 0;
> +}
> +
> +static inline long acpi_is_video_device(struct acpi_device *device)
> +{
> +       return 0;
> +}
> +
> +static inline int acpi_video_backlight_support(void)
> +{
> +       return 0;
> +}
> +
> +static inline int acpi_video_display_switch_support(void)
> +{
> +       return 0;
> +}
> +
> +#endif /* defined(CONFIG_ACPI_VIDEO) ||
> defined(CONFIG_ACPI_VIDEO_MODULE) */
> +
>  extern int acpi_blacklisted(void);
>  #ifdef CONFIG_DMI
>  extern void acpi_dmi_osi_linux(int enable, const struct dmi_system_id
> *d);
> --
> 1.5.4.5
> 
> 
> 

Re: [PATCH 02/10] Check for ACPI backlight support otherwise use vendor ACPI drivers

[Bjorn Helgaas]

On Friday 01 August 2008 09:37:55 am Thomas Renninger wrote:
> @@ -1013,7 +983,7 @@ static void acpi_device_set_id(struct acpi_device *device,
>  		   will get autoloaded and the device might still match
>  		   against another driver.
>  		*/
> -		if (ACPI_SUCCESS(acpi_video_bus_match(device)))
> +		if (acpi_is_video_device(device))
>  			cid_add = ACPI_VIDEO_HID;
>  		else if (ACPI_SUCCESS(acpi_bay_match(device)))
>  			cid_add = ACPI_BAY_HID;

It doesn't seem right to me to make this core behavior depend
on a config option.  With this approach, the ACPI device tree may
or may not contain an ACPI_VIDEO_HID device, depending on whether
CONFIG_ACPI_VIDEO is set, and that seems capricious.

What is the benefit of moving this code out of scan.c?  It's not
a very big function, and I think the consistency is worth the extra
code.

> +#if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
> +
> +extern long acpi_video_get_capabilities(acpi_handle graphics_dev_handle);
> +extern long acpi_is_video_device(struct acpi_device *device);
> +extern int acpi_video_backlight_support(void);
> +extern int acpi_video_display_switch_support(void);
> +
> +#else
> +
> +static inline long acpi_video_get_capabilities(acpi_handle graphics_dev_handle)
> +{
> +	return 0;
> +}
> +
> +static inline long acpi_is_video_device(struct acpi_device *device)
> +{
> +	return 0;
> +}

Re: [PATCH 02/10] Check for ACPI backlight support otherwise use vendor ACPI drivers

[Thomas Renninger]

On Friday 14 November 2008 04:27:42 pm Bjorn Helgaas wrote:
> On Friday 01 August 2008 09:37:55 am Thomas Renninger wrote:
> > @@ -1013,7 +983,7 @@ static void acpi_device_set_id(struct acpi_device
> > *device, will get autoloaded and the device might still match
> >  		   against another driver.
> >  		*/
> > -		if (ACPI_SUCCESS(acpi_video_bus_match(device)))
> > +		if (acpi_is_video_device(device))
> >  			cid_add = ACPI_VIDEO_HID;
> >  		else if (ACPI_SUCCESS(acpi_bay_match(device)))
> >  			cid_add = ACPI_BAY_HID;
>
> It doesn't seem right to me to make this core behavior depend
> on a config option.  With this approach, the ACPI device tree may
> or may not contain an ACPI_VIDEO_HID device, depending on whether
> CONFIG_ACPI_VIDEO is set, and that seems capricious.
>
> What is the benefit of moving this code out of scan.c?  It's not
> a very big function, and I think the consistency is worth the extra
> code.
This was done because the function is re-used later to detect which kind of
video functionalities the video device provides.
I somehow agree, but I don't see much of a disadvantage having this separated. 
You do not have the video device marked as such if CONFIG_ACPI_VIDEO is not 
compiled in, but it shouldn't hurt?

The two corresponding functions:
acpi_backlight_cap_match
acpi_is_video_device

might also go back to video.c again and get exported there. I can do that if 
you think it's worth it.

    Thomas
>
> > +#if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
> > +
> > +extern long acpi_video_get_capabilities(acpi_handle
> > graphics_dev_handle); +extern long acpi_is_video_device(struct
> > acpi_device *device);
> > +extern int acpi_video_backlight_support(void);
> > +extern int acpi_video_display_switch_support(void);
> > +
> > +#else
> > +
> > +static inline long acpi_video_get_capabilities(acpi_handle
> > graphics_dev_handle) +{
> > +	return 0;
> > +}
> > +
> > +static inline long acpi_is_video_device(struct acpi_device *device)
> > +{
> > +	return 0;
> > +}

Re: [PATCH 02/10] Check for ACPI backlight support otherwise use vendor ACPI drivers

[Bjorn Helgaas]

On Sunday 16 November 2008 03:07:31 pm Thomas Renninger wrote:
> On Friday 14 November 2008 04:27:42 pm Bjorn Helgaas wrote:
> > On Friday 01 August 2008 09:37:55 am Thomas Renninger wrote:
> > > @@ -1013,7 +983,7 @@ static void acpi_device_set_id(struct acpi_device
> > > *device, will get autoloaded and the device might still match
> > >  		   against another driver.
> > >  		*/
> > > -		if (ACPI_SUCCESS(acpi_video_bus_match(device)))
> > > +		if (acpi_is_video_device(device))
> > >  			cid_add = ACPI_VIDEO_HID;
> > >  		else if (ACPI_SUCCESS(acpi_bay_match(device)))
> > >  			cid_add = ACPI_BAY_HID;
> >
> > It doesn't seem right to me to make this core behavior depend
> > on a config option.  With this approach, the ACPI device tree may
> > or may not contain an ACPI_VIDEO_HID device, depending on whether
> > CONFIG_ACPI_VIDEO is set, and that seems capricious.
> >
> > What is the benefit of moving this code out of scan.c?  It's not
> > a very big function, and I think the consistency is worth the extra
> > code.
> This was done because the function is re-used later to detect which kind of
> video functionalities the video device provides.
> I somehow agree, but I don't see much of a disadvantage having this separated. 
> You do not have the video device marked as such if CONFIG_ACPI_VIDEO is not 
> compiled in, but it shouldn't hurt?

My personal opinion is that the device tree should be the same,
regardless of which drivers were selected at build-time.  It should
be possible to build a kernel with CONFIG_ACPI_VIDEO=n, then decide
later that you want to load a video driver.  Plus, it took me about
half an hour to figure out why the Makefile looks weird:

	obj-$(CONFIG_ACPI_VIDEO)        += video.o
	ifdef CONFIG_ACPI_VIDEO
	obj-y                           += video_detect.o
	endif

(I first thought video_detect.o could just be moved up to the
first line along with video.o.)

> The two corresponding functions:
> acpi_backlight_cap_match
> acpi_is_video_device
> 
> might also go back to video.c again and get exported there. I can do that if 
> you think it's worth it.

As long as we use the "self-defined HID" strategy for binding drivers,
I think those HIDs should be considered part of the core, and they
should be set whenever CONFIG_ACPI=y.  I don't think they should
depend on which drivers were selected at build-time.  In some ways,
it might be nicer if a driver could supply its own "match" function,
and then all the video-related special cases could be in the video
driver.

Bjorn

Re: [PATCH 02/10] Check for ACPI backlight support otherwise use vendor ACPI drivers

[Thomas Renninger]

On Monday 17 November 2008 08:51:42 pm Bjorn Helgaas wrote:
> On Sunday 16 November 2008 03:07:31 pm Thomas Renninger wrote:
> > On Friday 14 November 2008 04:27:42 pm Bjorn Helgaas wrote:
> > > On Friday 01 August 2008 09:37:55 am Thomas Renninger wrote:
> > > > @@ -1013,7 +983,7 @@ static void acpi_device_set_id(struct
> > > > acpi_device *device, will get autoloaded and the device might still
> > > > match against another driver.
> > > >  		*/
> > > > -		if (ACPI_SUCCESS(acpi_video_bus_match(device)))
> > > > +		if (acpi_is_video_device(device))
> > > >  			cid_add = ACPI_VIDEO_HID;
> > > >  		else if (ACPI_SUCCESS(acpi_bay_match(device)))
> > > >  			cid_add = ACPI_BAY_HID;
> > >
> > > It doesn't seem right to me to make this core behavior depend
> > > on a config option.  With this approach, the ACPI device tree may
> > > or may not contain an ACPI_VIDEO_HID device, depending on whether
> > > CONFIG_ACPI_VIDEO is set, and that seems capricious.
> > >
> > > What is the benefit of moving this code out of scan.c?  It's not
> > > a very big function, and I think the consistency is worth the extra
> > > code.
> >
> > This was done because the function is re-used later to detect which kind
> > of video functionalities the video device provides.
> > I somehow agree, but I don't see much of a disadvantage having this
> > separated. You do not have the video device marked as such if
> > CONFIG_ACPI_VIDEO is not compiled in, but it shouldn't hurt?
>
> My personal opinion is that the device tree should be the same,
> regardless of which drivers were selected at build-time.  It should
> be possible to build a kernel with CONFIG_ACPI_VIDEO=n, then decide
> later that you want to load a video driver.  Plus, it took me about
> half an hour to figure out why the Makefile looks weird:
>
> 	obj-$(CONFIG_ACPI_VIDEO)        += video.o
> 	ifdef CONFIG_ACPI_VIDEO
> 	obj-y                           += video_detect.o
> 	endif
>
> (I first thought video_detect.o could just be moved up to the
> first line along with video.o.)
>
> > The two corresponding functions:
> > acpi_backlight_cap_match
> > acpi_is_video_device
> >
> > might also go back to video.c again and get exported there. I can do that
> > if you think it's worth it.
>
> As long as we use the "self-defined HID" strategy for binding drivers,
> I think those HIDs should be considered part of the core, and they
> should be set whenever CONFIG_ACPI=y.  I don't think they should
> depend on which drivers were selected at build-time.  In some ways,
> it might be nicer if a driver could supply its own "match" function,
> and then all the video-related special cases could be in the video
> driver.

The video and other drivers cannot provide their own match function or
autoloading of the driver would not work.
Either you compile it into the core and the detection/matching will always 
take place or you do it as it is currently done and the matching for video 
devices will only be done if ACPI_VIDEO is compiled in.

      Thomas

[PATCH 03/10] Acer-WMI: fingers off backlight if video.ko is serving this functionality

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/misc/acer-wmi.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/misc/acer-wmi.c b/drivers/misc/acer-wmi.c
index e7a3fe5..a843ac2 100644
--- a/drivers/misc/acer-wmi.c
+++ b/drivers/misc/acer-wmi.c
@@ -1218,6 +1218,12 @@ static int __init acer_wmi_init(void)
 		return -ENODEV;
 	}
 
+	if (!acpi_video_backlight_support() && has_cap(ACER_CAP_BRIGHTNESS)) {
+		interface->capability &= ~ACER_CAP_BRIGHTNESS;
+		printk(ACER_INFO "Brightness must be controlled by "
+		       "generic video driver\n");
+	}
+
 	if (platform_driver_register(&acer_platform_driver)) {
 		printk(ACER_ERR "Unable to register platform driver.\n");
 		goto error_platform_register;
-- 
1.5.4.5

Re: [PATCH 03/10] Acer-WMI: fingers off backlight if video.ko is serving this functionality

[Carlos Corbacho]

On Friday 01 August 2008 16:37:56 Thomas Renninger wrote:
> Signed-off-by: Thomas Renninger <trenn@suse.de>

Acked-by: Carlos Corbacho <carlos@strangeworlds.co.uk>

> ---
>  drivers/misc/acer-wmi.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/misc/acer-wmi.c b/drivers/misc/acer-wmi.c
> index e7a3fe5..a843ac2 100644
> --- a/drivers/misc/acer-wmi.c
> +++ b/drivers/misc/acer-wmi.c
> @@ -1218,6 +1218,12 @@ static int __init acer_wmi_init(void)
>  		return -ENODEV;
>  	}
>
> +	if (!acpi_video_backlight_support() && has_cap(ACER_CAP_BRIGHTNESS)) {
> +		interface->capability &= ~ACER_CAP_BRIGHTNESS;
> +		printk(ACER_INFO "Brightness must be controlled by "
> +		       "generic video driver\n");
> +	}
> +
>  	if (platform_driver_register(&acer_platform_driver)) {
>  		printk(ACER_ERR "Unable to register platform driver.\n");
>  		goto error_platform_register;



-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

[PATCH 04/10] asus-acpi: fingers off backlight if video.ko is serving this functionality

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/misc/asus-laptop.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/asus-laptop.c b/drivers/misc/asus-laptop.c
index 7c6dfd0..a9a823b 100644
--- a/drivers/misc/asus-laptop.c
+++ b/drivers/misc/asus-laptop.c
@@ -1207,9 +1207,13 @@ static int __init asus_laptop_init(void)
 
 	dev = acpi_get_physical_device(hotk->device->handle);
 
-	result = asus_backlight_init(dev);
-	if (result)
-		goto fail_backlight;
+	if (!acpi_video_backlight_support()) {
+		result = asus_backlight_init(dev);
+		if (result)
+			goto fail_backlight;
+	} else
+		printk(ASUS_INFO "Brightness ignored, must be controlled by "
+		       "ACPI video driver\n");
 
 	result = asus_led_init(dev);
 	if (result)
-- 
1.5.4.5

[PATCH 05/10] compal: fingers off backlight if video.ko is serving this functionality

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/misc/compal-laptop.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/compal-laptop.c b/drivers/misc/compal-laptop.c
index 344b790..183dc3a 100644
--- a/drivers/misc/compal-laptop.c
+++ b/drivers/misc/compal-laptop.c
@@ -326,12 +326,14 @@ static int __init compal_init(void)
 
 	/* Register backlight stuff */
 
-	compalbl_device = backlight_device_register("compal-laptop", NULL, NULL,
-						&compalbl_ops);
-	if (IS_ERR(compalbl_device))
-		return PTR_ERR(compalbl_device);
-
-	compalbl_device->props.max_brightness = COMPAL_LCD_LEVEL_MAX-1;
+	if (!acpi_video_backlight_support()) {
+		compalbl_device = backlight_device_register("compal-laptop", NULL, NULL,
+							    &compalbl_ops);
+		if (IS_ERR(compalbl_device))
+			return PTR_ERR(compalbl_device);
+		
+		compalbl_device->props.max_brightness = COMPAL_LCD_LEVEL_MAX-1;
+	}
 
 	ret = platform_driver_register(&compal_driver);
 	if (ret)
-- 
1.5.4.5

[PATCH 06/10] eeepc-laptop: fingers off backlight if video.ko is serving this functionality

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/misc/eeepc-laptop.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/eeepc-laptop.c b/drivers/misc/eeepc-laptop.c
index 9e8d79e..0d891a6 100644
--- a/drivers/misc/eeepc-laptop.c
+++ b/drivers/misc/eeepc-laptop.c
@@ -625,9 +625,15 @@ static int __init eeepc_laptop_init(void)
 		return -ENODEV;
 	}
 	dev = acpi_get_physical_device(ehotk->device->handle);
-	result = eeepc_backlight_init(dev);
-	if (result)
-		goto fail_backlight;
+
+	if (!acpi_video_backlight_support()) {
+		result = eeepc_backlight_init(dev);
+		if (result)
+			goto fail_backlight;
+	} else
+		printk(EEEPC_INFO "Backlight controlled by ACPI video "
+		       "driver\n");
+
 	result = eeepc_hwmon_init(dev);
 	if (result)
 		goto fail_hwmon;
-- 
1.5.4.5

[PATCH 07/10] fujitsu-laptop: fingers off backlight if video.ko is serving this functionality

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/misc/fujitsu-laptop.c |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/misc/fujitsu-laptop.c b/drivers/misc/fujitsu-laptop.c
index 7a1ef6c..e72a9ac 100644
--- a/drivers/misc/fujitsu-laptop.c
+++ b/drivers/misc/fujitsu-laptop.c
@@ -963,16 +963,16 @@ static int __init fujitsu_init(void)
 
 	/* Register backlight stuff */
 
-	fujitsu->bl_device =
-	    backlight_device_register("fujitsu-laptop", NULL, NULL,
-				      &fujitsubl_ops);
-	if (IS_ERR(fujitsu->bl_device))
-		return PTR_ERR(fujitsu->bl_device);
-
-	max_brightness = fujitsu->max_brightness;
-
-	fujitsu->bl_device->props.max_brightness = max_brightness - 1;
-	fujitsu->bl_device->props.brightness = fujitsu->brightness_level;
+	if (!acpi_video_backlight_support()) {
+		fujitsu->bl_device =
+			backlight_device_register("fujitsu-laptop", NULL, NULL,
+						  &fujitsubl_ops);
+		if (IS_ERR(fujitsu->bl_device))
+			return PTR_ERR(fujitsu->bl_device);
+		max_brightness = fujitsu->max_brightness;
+		fujitsu->bl_device->props.max_brightness = max_brightness - 1;
+		fujitsu->bl_device->props.brightness = fujitsu->brightness_level;
+	}
 
 	ret = platform_driver_register(&fujitsupf_driver);
 	if (ret)
@@ -1008,7 +1008,8 @@ fail_hotkey:
 
 fail_backlight:
 
-	backlight_device_unregister(fujitsu->bl_device);
+	if (fujitsu->bl_device)
+		backlight_device_unregister(fujitsu->bl_device);
 
 fail_platform_device2:
 
@@ -1035,7 +1036,8 @@ static void __exit fujitsu_cleanup(void)
 			   &fujitsupf_attribute_group);
 	platform_device_unregister(fujitsu->pf_device);
 	platform_driver_unregister(&fujitsupf_driver);
-	backlight_device_unregister(fujitsu->bl_device);
+	if (fujitsu->bl_device)
+		backlight_device_unregister(fujitsu->bl_device);
 
 	acpi_bus_unregister_driver(&acpi_fujitsu_driver);
 
-- 
1.5.4.5

[PATCH 08/10] msi-laptop: fingers off backlight if video.ko is serving this functionality

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/misc/msi-laptop.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/msi-laptop.c b/drivers/misc/msi-laptop.c
index de898c6..759763d 100644
--- a/drivers/misc/msi-laptop.c
+++ b/drivers/misc/msi-laptop.c
@@ -347,12 +347,16 @@ static int __init msi_init(void)
 
 	/* Register backlight stuff */
 
-	msibl_device = backlight_device_register("msi-laptop-bl", NULL, NULL,
-						&msibl_ops);
-	if (IS_ERR(msibl_device))
-		return PTR_ERR(msibl_device);
-
-	msibl_device->props.max_brightness = MSI_LCD_LEVEL_MAX-1;
+	if (acpi_video_backlight_support()) {
+		printk(KERN_INFO "MSI: Brightness ignored, must be controlled "
+		       "by ACPI video driver\n");
+	} else {
+		msibl_device = backlight_device_register("msi-laptop-bl", NULL,
+							 NULL, &msibl_ops);
+		if (IS_ERR(msibl_device))
+			return PTR_ERR(msibl_device);
+		msibl_device->props.max_brightness = MSI_LCD_LEVEL_MAX-1;
+	}
 
 	ret = platform_driver_register(&msipf_driver);
 	if (ret)
-- 
1.5.4.5

[PATCH 09/10] sony-laptop: fingers off backlight if video.ko is serving this functionality

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/misc/sony-laptop.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/misc/sony-laptop.c b/drivers/misc/sony-laptop.c
index 60775be..3d178ab 100644
--- a/drivers/misc/sony-laptop.c
+++ b/drivers/misc/sony-laptop.c
@@ -1038,7 +1038,11 @@ static int sony_nc_add(struct acpi_device *device)
 		goto outinput;
 	}
 
-	if (ACPI_SUCCESS(acpi_get_handle(sony_nc_acpi_handle, "GBRT", &handle))) {
+	if (!acpi_video_backlight_support()) {
+		printk(KERN_INFO DRV_PFX "Sony: Brightness ignored, must be "
+		       "controlled by ACPI video driver\n");
+	} else if (ACPI_SUCCESS(acpi_get_handle(sony_nc_acpi_handle, "GBRT",
+						&handle))) {
 		sony_backlight_device = backlight_device_register("sony", NULL,
 								  NULL,
 								  &sony_backlight_ops);
-- 
1.5.4.5

[PATCH 10/10] thinkpad_acpi: fingers off backlight if video.ko is serving this functionality

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/misc/thinkpad_acpi.c |   31 ++++++++++++++++++++-----------
 1 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index d3eb790..a71ecd2 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -4922,17 +4922,26 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
 	 */
 	b = tpacpi_check_std_acpi_brightness_support();
 	if (b > 0) {
-		if (thinkpad_id.vendor == PCI_VENDOR_ID_LENOVO) {
-			printk(TPACPI_NOTICE
-			       "Lenovo BIOS switched to ACPI backlight "
-			       "control mode\n");
-		}
-		if (brightness_enable > 1) {
-			printk(TPACPI_NOTICE
-			       "standard ACPI backlight interface "
-			       "available, not loading native one...\n");
-			return 1;
-		}
+
+		if (acpi_video_backlight_support()) {
+			if (brightness_enable > 1) {
+				printk(TPACPI_NOTICE
+				       "Standard ACPI backlight interface "
+				       "available, not loading native one.\n");
+				return 1;
+			} else if (brightness_enable == 1) {
+				printk(TPACPI_NOTICE
+				       "Backlight control force, even standard "
+				       "ACPI backlight interface available\n");
+			}
+		} else {
+			if (brightness_enable > 1) {
+				printk(TPACPI_NOTICE
+				       "Standard ACPI backlight interface not "
+				       "available, thinkpad_acpi driver "
+				       "will take over control\n");
+			}
+ 		}
 	}
 
 	if (!brightness_enable) {
-- 
1.5.4.5

Re: [PATCH 10/10] thinkpad_acpi: fingers off backlight if video.ko is serving this functionality

[Henrique de Moraes Holschuh]

On Fri, 01 Aug 2008, Thomas Renninger wrote:
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> ---
>  drivers/misc/thinkpad_acpi.c |   31 ++++++++++++++++++++-----------
>  1 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> index d3eb790..a71ecd2 100644
> --- a/drivers/misc/thinkpad_acpi.c
> +++ b/drivers/misc/thinkpad_acpi.c
> @@ -4922,17 +4922,26 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
>  	 */
>  	b = tpacpi_check_std_acpi_brightness_support();
>  	if (b > 0) {
> -		if (thinkpad_id.vendor == PCI_VENDOR_ID_LENOVO) {
> -			printk(TPACPI_NOTICE
> -			       "Lenovo BIOS switched to ACPI backlight "
> -			       "control mode\n");
> -		}
> -		if (brightness_enable > 1) {
> -			printk(TPACPI_NOTICE
> -			       "standard ACPI backlight interface "
> -			       "available, not loading native one...\n");
> -			return 1;
> -		}
> +
> +		if (acpi_video_backlight_support()) {
> +			if (brightness_enable > 1) {
> +				printk(TPACPI_NOTICE
> +				       "Standard ACPI backlight interface "
> +				       "available, not loading native one.\n");
> +				return 1;
> +			} else if (brightness_enable == 1) {
> +				printk(TPACPI_NOTICE
> +				       "Backlight control force, even standard "
> +				       "ACPI backlight interface available\n");
> +			}

EPARSE.  "Backlight control force, even standard ACPI backlight interface
available" is no good.  Maybe you want to change that to "Backlight control
force enabled, even if standard ACPI backlight interface is available" ?

> +		} else {
> +			if (brightness_enable > 1) {
> +				printk(TPACPI_NOTICE
> +				       "Standard ACPI backlight interface not "
> +				       "available, thinkpad_acpi driver "
> +				       "will take over control\n");

"thinkpad-acpi native brightness control enabled", perhaps?

> +			}
> + 		}
>  	}
>  
>  	if (!brightness_enable) {

Provided that you clarify the printk I marked with EPARSE:
Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

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

Re: [PATCH 10/10] thinkpad_acpi: fingers off backlight if video.ko is serving this functionality

[Len Brown]

On Fri, 1 Aug 2008, Henrique de Moraes Holschuh wrote:

> On Fri, 01 Aug 2008, Thomas Renninger wrote:
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > ---
> >  drivers/misc/thinkpad_acpi.c |   31 ++++++++++++++++++++-----------
> >  1 files changed, 20 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> > index d3eb790..a71ecd2 100644
> > --- a/drivers/misc/thinkpad_acpi.c
> > +++ b/drivers/misc/thinkpad_acpi.c
> > @@ -4922,17 +4922,26 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
> >  	 */
> >  	b = tpacpi_check_std_acpi_brightness_support();
> >  	if (b > 0) {
> > -		if (thinkpad_id.vendor == PCI_VENDOR_ID_LENOVO) {
> > -			printk(TPACPI_NOTICE
> > -			       "Lenovo BIOS switched to ACPI backlight "
> > -			       "control mode\n");
> > -		}
> > -		if (brightness_enable > 1) {
> > -			printk(TPACPI_NOTICE
> > -			       "standard ACPI backlight interface "
> > -			       "available, not loading native one...\n");
> > -			return 1;
> > -		}
> > +
> > +		if (acpi_video_backlight_support()) {
> > +			if (brightness_enable > 1) {
> > +				printk(TPACPI_NOTICE
> > +				       "Standard ACPI backlight interface "
> > +				       "available, not loading native one.\n");
> > +				return 1;
> > +			} else if (brightness_enable == 1) {
> > +				printk(TPACPI_NOTICE
> > +				       "Backlight control force, even standard "
> > +				       "ACPI backlight interface available\n");
> > +			}
> 
> EPARSE.  "Backlight control force, even standard ACPI backlight interface
> available" is no good.  Maybe you want to change that to "Backlight control
> force enabled, even if standard ACPI backlight interface is available" ?

updated.

> > +		} else {
> > +			if (brightness_enable > 1) {
> > +				printk(TPACPI_NOTICE
> > +				       "Standard ACPI backlight interface not "
> > +				       "available, thinkpad_acpi driver "
> > +				       "will take over control\n");
> 
> "thinkpad-acpi native brightness control enabled", perhaps?

updated.

> > +			}
> > + 		}
> >  	}
> >  
> >  	if (!brightness_enable) {
> 
> Provided that you clarify the printk I marked with EPARSE:
> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

thanks,
-Len

Re: Check for ACPI backlight support otherwise use vendor ACPI drivers - version 4

[Andi Kleen]

Added to the test tree thanks 
(git://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-acpi-2.6.git test)

-Andi

Do not return true to all kind of Windows OSI calls

[Thomas Renninger]

ThinkPads store OSI calls in separate variables and later
check for NT/XP/VISTA specific code to process.

I lately saw a line on a ThinkPad checking for XP and VISTA
at video device specific code.

So currently some ThinkPads run into code paths which should
either run on XP or VISTA, but Linux might execute them both
or in a way that is not intended by BIOS developers.

Also be sure you have a recent BIOS running.

       Thomas

Re: Do not return true to all kind of Windows OSI calls

[Matthew Garrett]

On Sat, Aug 02, 2008 at 06:57:57PM +0200, Thomas Renninger wrote:
> ThinkPads store OSI calls in separate variables and later
> check for NT/XP/VISTA specific code to process.
> 
> I lately saw a line on a ThinkPad checking for XP and VISTA
> at video device specific code.
> 
> So currently some ThinkPads run into code paths which should
> either run on XP or VISTA, but Linux might execute them both
> or in a way that is not intended by BIOS developers.

Could you please show the code in question? I think you're misdiagnosing 
the issue.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Do not return true to all kind of Windows OSI calls

[Thomas Renninger]

On Saturday 02 August 2008 06:59:52 pm Matthew Garrett wrote:
> On Sat, Aug 02, 2008 at 06:57:57PM +0200, Thomas Renninger wrote:
> > ThinkPads store OSI calls in separate variables and later
> > check for NT/XP/VISTA specific code to process.
> >
> > I lately saw a line on a ThinkPad checking for XP and VISTA
> > at video device specific code.
> >
> > So currently some ThinkPads run into code paths which should
> > either run on XP or VISTA, but Linux might execute them both
> > or in a way that is not intended by BIOS developers.
>
> Could you please show the code in question? I think you're misdiagnosing
> the issue.


If (CondRefOf (\_OSI, Local0))
                {
                    If (\_OSI ("Windows 2001"))
                    {
                        Store (0x01, \WNTF)
                        Store (0x01, \WXPF)
                        Store (0x00, \WSPV)
                    }

                    If (\_OSI ("Windows 2001 SP1"))
                    {
                        Store (0x01, \WSPV)
                    }

                    If (\_OSI ("Windows 2001 SP2"))
                    {
                        Store (0x02, \WSPV)
                    }

                    If (\_OSI ("Windows 2006"))
                    {
                        Store (0x01, \WVIS)
                    }
                }

...
# Only process this on XP, not on Linux and Vista
                           If (LAnd (\WXPF, LNot (\WVIS)))
                            {
                                Notify (\_SB.PCI0, Arg1)
                            }
                            Else
                            {
                                Notify (\_SB.PCI0.VID, Arg1)
                            }
                        }
...

# Only process this on XP, not on Linux and Vista
                        If (LAnd (LNot (\WXPF), \WNTF))
                        {
                            Store (0x00, \_SB.PCI0.LPC.C4C3)
                        }
...
                Device (HPET)
                {
                    Name (_HID, EisaId ("PNP0103"))
                    Method (_STA, 0, NotSerialized)
                    {
  ...
# Only disable HPET on NT, not on Linux
                          If (LAnd (\WNTF, LNot (\WXPF)))
                            {
                                Return (0x00)
                            }


Amazing, it looks like they already check for Linux by checking if
OSI returned true on two major Windows versions which would indicate
Linux running.

So yes, everything looks fine and it seems this was not luck, but
they already realized Linux does not return true for OSI(Linux)
anymore.

Why must we force vendors to implement such a madness, the OSI
implementation is so wrong.

So this seems to get the state-of-the-art then..., check whether several
Windows versions return true and you do not need to process broken code
which is only intended to run on a very specific broken Windows version.

     Thomas

Re: Do not return true to all kind of Windows OSI calls

[Matthew Garrett]

On Sat, Aug 02, 2008 at 06:48:15PM +0200, Thomas Renninger wrote:
> On Saturday 02 August 2008 06:59:52 pm Matthew Garrett wrote:
> # Only process this on XP, not on Linux and Vista
>                            If (LAnd (\WXPF, LNot (\WVIS)))

The only way this statement makes sense is if both WXPF and WVIS can be 
true, which is only possible if Vista responds to all of the _OSI 
requests.

> # Only process this on XP, not on Linux and Vista
>                         If (LAnd (LNot (\WXPF), \WNTF))

Actually, this is "Only execute on 2000, not on XP or higher".

> Amazing, it looks like they already check for Linux by checking if
> OSI returned true on two major Windows versions which would indicate
> Linux running.

No, that's not true at all. You haven't shown any evidence that Windows 
only returns true to a single OSI string. In fact, doing so would tend 
to break BIOSes that haven't yet been updated for newer versions of 
Windows.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Do not return true to all kind of Windows OSI calls

[Thomas Renninger]

On Saturday 02 August 2008 07:31:46 pm Matthew Garrett wrote:
> On Sat, Aug 02, 2008 at 06:48:15PM +0200, Thomas Renninger wrote:
> > On Saturday 02 August 2008 06:59:52 pm Matthew Garrett wrote:
> > # Only process this on XP, not on Linux and Vista
> >                            If (LAnd (\WXPF, LNot (\WVIS)))
>
> The only way this statement makes sense is if both WXPF and WVIS can be
> true, which is only possible if Vista responds to all of the _OSI
> requests.
No.
The machines are Linux pre-loaded. They do support Linux!
There is no need to check for both variables for Windows specific code (
there are tons of \WVIS and \WXPF only checking).
The only way this statement makes sense is that they *do* check for Linux.

> > # Only process this on XP, not on Linux and Vista
> >                         If (LAnd (LNot (\WXPF), \WNTF))
>
> Actually, this is "Only execute on 2000, not on XP or higher".
Yes I mixed up XP and NT.


       Thomas

Re: Do not return true to all kind of Windows OSI calls

[Matthew Garrett]

http://www.microsoft.com/whdc/archive/_OSI-method.mspx :

"Any follow-on operating system from Microsoft that can support the 
Windows XP feature set will also return TRUE for Windows 2001. For 
example, if Microsoft released a follow-on operating system in 2003, it 
will probably be assigned a string of Windows 2003. This operating 
system would then return TRUE for the strings Windows 2001 and Windows 
2003, and it would return FALSE for all other strings."

So, no, it is not a test for Linux. Windows reports all versions, not 
just the current one.
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Do not return true to all kind of Windows OSI calls

[Henrique de Moraes Holschuh]

On Sat, 02 Aug 2008, Matthew Garrett wrote:
> http://www.microsoft.com/whdc/archive/_OSI-method.mspx :
> 
> "Any follow-on operating system from Microsoft that can support the 
> Windows XP feature set will also return TRUE for Windows 2001. For 
> example, if Microsoft released a follow-on operating system in 2003, it 
> will probably be assigned a string of Windows 2003. This operating 
> system would then return TRUE for the strings Windows 2001 and Windows 
> 2003, and it would return FALSE for all other strings."
> 
> So, no, it is not a test for Linux. Windows reports all versions, not 
> just the current one.

I'd be careful with that "all versions".  The text only says XP onwards. We
claim OSI strings older than XP along with XP's and newer ones.

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

Re: Do not return true to all kind of Windows OSI calls

[Matthew Garrett]

On Sun, Aug 03, 2008 at 01:43:53AM -0300, Henrique de Moraes Holschuh wrote:
> On Sat, 02 Aug 2008, Matthew Garrett wrote:
> I'd be careful with that "all versions".  The text only says XP onwards. We
> claim OSI strings older than XP along with XP's and newer ones.

As far as I can tell, XP was the first to return OSI. Older versions of 
Windows only implemented _OS.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Do not return true to all kind of Windows OSI calls

[Thomas Renninger]

On Saturday 02 August 2008 09:48:43 pm Matthew Garrett wrote:
> http://www.microsoft.com/whdc/archive/_OSI-method.mspx :
>
> "Any follow-on operating system from Microsoft that can support the
> Windows XP feature set will also return TRUE for Windows 2001. For
> example, if Microsoft released a follow-on operating system in 2003, it
> will probably be assigned a string of Windows 2003. This operating
> system would then return TRUE for the strings Windows 2001 and Windows
> 2003, and it would return FALSE for all other strings."
>
> So, no, it is not a test for Linux. Windows reports all versions, not
> just the current one.

Yes that seems to be right.
Later versions explicitly check for linux/bsd:

                   If (\_OSI ("Windows 2006"))
                    {
                        Store (0x01, \WVIS)
                    }

                    If (\_OSI ("Linux"))
                    {
                        Store (0x01, \LNUX)
                    }

                    If (\_OSI ("FreeBSD"))
                    {
                        Store (0x01, \LNUX)
                    }

This is even worse, because that means that Lenovo and everyone
else who has to add Windows hotfixes to their BIOS and potentially
have to break their Linux supported OS by that cannot guarantee
their customers full Linux support (without providing a separate
Linux BIOS).

        Thomas

Re: Do not return true to all kind of Windows OSI calls

[Matthew Garrett]

On Sun, Aug 03, 2008 at 03:31:53PM +0200, Thomas Renninger wrote:

> This is even worse, because that means that Lenovo and everyone
> else who has to add Windows hotfixes to their BIOS and potentially
> have to break their Linux supported OS by that cannot guarantee
> their customers full Linux support (without providing a separate
> Linux BIOS).

That's fine. Where we diverge from the Windows behaviour we already have 
a bug.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Do not return true to all kind of Windows OSI calls

[Thomas Renninger]

On Sunday 03 August 2008 04:12:31 pm Matthew Garrett wrote:
> On Sun, Aug 03, 2008 at 03:31:53PM +0200, Thomas Renninger wrote:
> > This is even worse, because that means that Lenovo and everyone
> > else who has to add Windows hotfixes to their BIOS and potentially
> > have to break their Linux supported OS by that cannot guarantee
> > their customers full Linux support (without providing a separate
> > Linux BIOS).
>
> That's fine. Where we diverge from the Windows behaviour we already have
> a bug.

Yes.
I take above statement back.
This Windows behaviour very much de-escalates the situation.

If Windows sticks to this then there is no urgent need for OSI(notWindows)
or OSI(Linux).

Thanks for pointing this out.

       Thomas

RE: Do not return true to all kind of Windows OSI calls

[Moore, Robert]

We ran _OSI on a Windows XP machine with the following results:

>
>Name(buf, Buffer() {0, 0, 0, 0, 0, 0, 0, 0})
>Store(\_OSI("Windows 2000"), Index(buf, 0))
>Store(\_OSI("Windows 2001"), Index(buf, 1))
>Store(\_OSI("Windows 2001.1"), Index(buf, 2))
>Store(\_OSI("Windows 2001.1 SP1"), Index(buf, 3))
>Store(\_OSI("Windows 2001 SP1"), Index(buf, 4))
>Store(\_OSI("Windows 2001 SP2"), Index(buf, 5))
>Store(\_OSI("Windows 2001 SP3"), Index(buf, 6))
>Store(\_OSI("Windows 2006"), Index(buf, 7))
>
>The result is:
>Buffer: 0xff 0xff 0x00 0x00 0xff 0xff 0x00 0x00
>
>So "Windows 2000", "Windows 2001", "Windows 2001 SP1", "Windows 2001
SP2"
>are supported.
>
>Lin Ming





>-----Original Message-----
>From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>owner@vger.kernel.org] On Behalf Of Thomas Renninger
>Sent: Sunday, August 03, 2008 9:24 AM
>To: Matthew Garrett
>Cc: corsac@debian.org; ak@linux.intel.com; Zhang, Rui; nokos@gmx.net;
>linux-acpi@vger.kernel.org; hmh@debian.org
>Subject: Re: Do not return true to all kind of Windows OSI calls
>
>On Sunday 03 August 2008 04:12:31 pm Matthew Garrett wrote:
>> On Sun, Aug 03, 2008 at 03:31:53PM +0200, Thomas Renninger wrote:
>> > This is even worse, because that means that Lenovo and everyone
>> > else who has to add Windows hotfixes to their BIOS and potentially
>> > have to break their Linux supported OS by that cannot guarantee
>> > their customers full Linux support (without providing a separate
>> > Linux BIOS).
>>
>> That's fine. Where we diverge from the Windows behaviour we already
have
>> a bug.
>
>Yes.
>I take above statement back.
>This Windows behaviour very much de-escalates the situation.
>
>If Windows sticks to this then there is no urgent need for
OSI(notWindows)
>or OSI(Linux).
>
>Thanks for pointing this out.
>
>       Thomas
>--
>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

[PATCH 1/2] ACPI: Provide a OSI interface that does not return true for Windows

[Thomas Renninger]

Introduce acpi_osi=windows_false boot parameter
This will return false to _OSI("Windows XY") calls.

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 Documentation/kernel-parameters.txt |    1 +
 drivers/acpi/osl.c                  |   15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 0fea7c2..c1e397e 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -182,6 +182,7 @@ and is between 256 and 4096 characters. It is defined in the file
 	acpi_osi=	[HW,ACPI] Modify list of supported OS interface strings
 			acpi_osi="string1"	# add string1 -- only one string
 			acpi_osi="!string2"	# remove built-in string2
+			acpi_osi="windows_false"# remove all Windows strings
 			acpi_osi=		# disable all strings
 
 	acpi_serialize	[HW,ACPI] force serialization of AML methods
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 235a138..bf42ef1 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -943,7 +943,7 @@ static void __init set_osi_linux(unsigned int enable)
 	if (osi_linux.enable != enable) {
 		osi_linux.enable = enable;
 		printk(KERN_NOTICE PREFIX "%sed _OSI(Linux)\n",
-			enable ? "Add": "Delet");
+			enable ? "Add": "Delete");
 	}
 	return;
 }
@@ -972,6 +972,17 @@ void __init acpi_dmi_osi_linux(int enable, const struct dmi_system_id *d)
 	return;
 }
 
+static void __init acpi_osi_windows_false(void) {
+
+	acpi_osi_invalidate("Windows 2000");
+	acpi_osi_invalidate("Windows 2001");
+	acpi_osi_invalidate("Windows 2001 SP1");
+	acpi_osi_invalidate("Windows 2001 SP2");
+	acpi_osi_invalidate("Windows 2001.1");
+	acpi_osi_invalidate("Windows 2001.1 SP1");
+	acpi_osi_invalidate("Windows 2006");
+}
+
 /*
  * Modify the list of "OS Interfaces" reported to BIOS via _OSI
  *
@@ -984,6 +995,8 @@ int __init acpi_osi_setup(char *str)
 	if (str == NULL || *str == '\0') {
 		printk(KERN_INFO PREFIX "_OSI method disabled\n");
 		acpi_gbl_create_osi_method = FALSE;
+	} else if (strcmp("windows_false", str)) {
+		acpi_osi_windows_false();
 	} else if (!strcmp("!Linux", str)) {
 		acpi_cmdline_osi_linux(0);	/* !enable */
 	} else if (*str == '!') {
-- 
1.5.4.5

[PATCH 2/2] patch acpi_lenovo_thinkpad_only_return_vista.patch

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/acpi/osl.c                     |   11 +++++++++++
 drivers/net/wireless/iwlwifi/iwl-led.c |    2 --
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index bf42ef1..0d9e174 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -201,6 +201,17 @@ acpi_status __init acpi_os_initialize(void)
 
 acpi_status acpi_os_initialize1(void)
 {
+	/* Only return Vista 2006 for Lenovo ThinkPads */
+	if (dmi_name_in_vendors("LENOVO")) {
+		acpi_osi_invalidate("Windows 2000");
+		acpi_osi_invalidate("Windows 2001");
+		acpi_osi_invalidate("Windows 2001 SP1");
+		acpi_osi_invalidate("Windows 2001 SP2");
+		acpi_osi_invalidate("Windows 2001.1");
+		acpi_osi_invalidate("Windows 2001.1 SP1");
+		/* acpi_osi_invalidate("Windows 2006"); */
+	}
+
 	kacpid_wq = create_singlethread_workqueue("kacpid");
 	kacpi_notify_wq = create_singlethread_workqueue("kacpi_notify");
 	BUG_ON(!kacpid_wq);
diff --git a/drivers/net/wireless/iwlwifi/iwl-led.c b/drivers/net/wireless/iwlwifi/iwl-led.c
index 899d7a2..34de711 100644
--- a/drivers/net/wireless/iwlwifi/iwl-led.c
+++ b/drivers/net/wireless/iwlwifi/iwl-led.c
@@ -195,8 +195,6 @@ static void iwl_led_brightness_set(struct led_classdev *led_cdev,
 		return;
 
 
-	IWL_DEBUG_LED("Led type = %s brightness = %d\n",
-			led_type_str[led->type], brightness);
 	switch (brightness) {
 	case LED_FULL:
 		if (led->type == IWL_LED_TRG_ASSOC)
-- 
1.5.4.5

Re: [PATCH 2/2] patch acpi_lenovo_thinkpad_only_return_vista.patch

[Thomas Renninger]

On Saturday 02 August 2008 06:57:59 pm Thomas Renninger wrote:
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> ---
>  drivers/acpi/osl.c                     |   11 +++++++++++
>  drivers/net/wireless/iwlwifi/iwl-led.c |    2 --
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index bf42ef1..0d9e174 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -201,6 +201,17 @@ acpi_status __init acpi_os_initialize(void)
>
>  acpi_status acpi_os_initialize1(void)
>  {
> +	/* Only return Vista 2006 for Lenovo ThinkPads */
> +	if (dmi_name_in_vendors("LENOVO")) {
> +		acpi_osi_invalidate("Windows 2000");
> +		acpi_osi_invalidate("Windows 2001");
> +		acpi_osi_invalidate("Windows 2001 SP1");
> +		acpi_osi_invalidate("Windows 2001 SP2");
> +		acpi_osi_invalidate("Windows 2001.1");
> +		acpi_osi_invalidate("Windows 2001.1 SP1");
> +		/* acpi_osi_invalidate("Windows 2006"); */
> +	}
> +
>  	kacpid_wq = create_singlethread_workqueue("kacpid");
>  	kacpi_notify_wq = create_singlethread_workqueue("kacpi_notify");
>  	BUG_ON(!kacpid_wq);

Ops, the stuff below is unrelated.

> diff --git a/drivers/net/wireless/iwlwifi/iwl-led.c
> b/drivers/net/wireless/iwlwifi/iwl-led.c index 899d7a2..34de711 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-led.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-led.c
> @@ -195,8 +195,6 @@ static void iwl_led_brightness_set(struct led_classdev
> *led_cdev, return;
>
>
> -	IWL_DEBUG_LED("Led type = %s brightness = %d\n",
> -			led_type_str[led->type], brightness);
>  	switch (brightness) {
>  	case LED_FULL:
>  		if (led->type == IWL_LED_TRG_ASSOC)

Also the first patch probably disables video.ko brightness switching and 
thinkpad_acpi has to be used then. The way it worked before Vista...

But only returning true to Windows 2006, is worth a try IMO.

       Thomas

Re: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Zhang Rui]

On Sat, 2008-08-02 at 07:20 +0800, Yves-Alexis Perez wrote:
> On ven, 2008-08-01 at 17:37 +0200, Thomas Renninger wrote:
> > Please queue for 2.6.28.
> > Tell me if parts do not patch and I can repost them against
> > whatever is needed.
> > 
> > Ideally they should go in together with Matthew's/Hong's IGD
> > patches (which should come mainline through the dri tree), these
> > patches should need mine.
> 
> Ok, as those patches were merged in linux-acpi-2.6/test I managed to
> build and test (0d0b6e44c2d3295b790ecfd20d2461181e10c846).
> 
> I'm using a Thinkpad T61 with Intel graphics. Config is attached.
> 
> When booting in single user, without acpid, hal, dbus etc. running,
> brightness keys doesn't work. thinkpad-acpi and video are loaded, I
> tried to boot with acpi_backlight=video just to be sure, but nothing
> changes. I have only one device in /sys/class/backlight (acpi_video0).
> brightness and actual_brightness follow the changes when I press the
> brigthness keys, but the real brightness doesnt.

I think you're using the laptop that this patch does break.
Please try acpi_backlight=vendor.

thanks,
rui

Re: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Yves-Alexis Perez]

[-- Attachment #1: Type: text/plain, Size: 2763 bytes --]

On lun, 2008-08-04 at 09:16 +0800, Zhang Rui wrote:
> I think you're using the laptop that this patch does break.
> Please try acpi_backlight=vendor.

When booting to single user with acpi_backlight=vendor, thinkpad-acpi
takes over backlight:

Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: ThinkPad ACPI Extras v0.21
Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: http://ibm-acpi.sf.net/
Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: ThinkPad BIOS 7LETB9WW (2.19 ), EC 7KHT24WW-1.08
Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: Lenovo ThinkPad T61, model 8897CTO
Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: radio switch found; radios are enabled
Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: This ThinkPad has standard ACPI backlight brightness control, supported by the ACPI video driver
Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: Disabling thinkpad-acpi brightness events by default...
Aug  4 07:54:38 hidalgo kernel: Registered led device: tpacpi::thinklight
Aug  4 07:54:38 hidalgo kernel: Registered led device: tpacpi::power
Aug  4 07:54:38 hidalgo kernel: Registered led device: tpacpi:orange:batt
Aug  4 07:54:38 hidalgo kernel: Registered led device: tpacpi:green:batt
Aug  4 07:54:38 hidalgo kernel: Registered led device: tpacpi::dock_active
Aug  4 07:54:38 hidalgo kernel: Registered led device: tpacpi::bay_active
Aug  4 07:54:38 hidalgo kernel: Registered led device: tpacpi::dock_batt
Aug  4 07:54:38 hidalgo kernel: Registered led device: tpacpi::unknown_led
Aug  4 07:54:38 hidalgo kernel: Registered led device: tpacpi::standby
Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: Standard ACPI backlight interface not available, thinkpad_acpi driver will take over control
Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: detected a 16-level brightness capable ThinkPad
Aug  4 07:54:38 hidalgo kernel: input: ThinkPad Extra Buttons as /devices/virtual/input/input6
Aug  4 07:54:38 hidalgo kernel: usb 1-1: new full speed USB device using uhci_hcd and address 4
Aug  4 07:54:38 hidalgo kernel: usb 1-1: configuration #1 chosen from 1 choice

But when in user mode, brightness keys doesn't do anything either.
Echo'ing to /proc/acpi/ibm/brightness doesn't work, but echoing
to /sys/class/backlight/thinkpad_screen/brightness works.

Cat'ing /proc/acpi/events still gives:
video LCD0 00000086 00000000
video LCD0 00000087 00000000

At one time I get the 750ms delay, but I don't have the 
ibm/hotkey HKEY 00000080 00005010 at all.

In X, same thing, no brightness keys, but xbacklight does work in
standard mode (“kernel”).

(btw it seems my mails don't reach linux-acpi, I don't really know why
and postmaster don't answer, maybe they didn't have my mail either).

Cheers,
-- 
Yves-Alexis

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Yves-Alexis Perez]

[-- Attachment #1: Type: text/plain, Size: 297 bytes --]

On lun, 2008-08-04 at 08:09 +0200, Yves-Alexis Perez wrote:
> (btw it seems my mails don't reach linux-acpi, I don't really know why
> and postmaster don't answer, maybe they didn't have my mail either).

Well, at least this one manage to went through.
Sorry for the noise
-- 
Yves-Alexis

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Thomas Renninger]

On Monday 04 August 2008 08:09:52 Yves-Alexis Perez wrote:
> On lun, 2008-08-04 at 09:16 +0800, Zhang Rui wrote:
> > I think you're using the laptop that this patch does break.
> > Please try acpi_backlight=vendor.
>
> When booting to single user with acpi_backlight=vendor, thinkpad-acpi
> takes over backlight:
>
> Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: ThinkPad ACPI Extras v0.21
> Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: http://ibm-acpi.sf.net/
> Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: ThinkPad BIOS 7LETB9WW (2.19
> ), EC 7KHT24WW-1.08 Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: Lenovo
> ThinkPad T61, model 8897CTO Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi:
> radio switch found; radios are enabled Aug  4 07:54:38 hidalgo kernel:
> thinkpad_acpi: This ThinkPad has standard ACPI backlight brightness
> control, supported by the ACPI video driver Aug  4 07:54:38 hidalgo kernel:
> thinkpad_acpi: Disabling thinkpad-acpi brightness events by default... Aug 
> 4 07:54:38 hidalgo kernel: Registered led device: tpacpi::thinklight Aug  4
> 07:54:38 hidalgo kernel: Registered led device: tpacpi::power Aug  4
> 07:54:38 hidalgo kernel: Registered led device: tpacpi:orange:batt Aug  4
> 07:54:38 hidalgo kernel: Registered led device: tpacpi:green:batt Aug  4
> 07:54:38 hidalgo kernel: Registered led device: tpacpi::dock_active Aug  4
> 07:54:38 hidalgo kernel: Registered led device: tpacpi::bay_active Aug  4
> 07:54:38 hidalgo kernel: Registered led device: tpacpi::dock_batt Aug  4
> 07:54:38 hidalgo kernel: Registered led device: tpacpi::unknown_led Aug  4
> 07:54:38 hidalgo kernel: Registered led device: tpacpi::standby Aug  4
> 07:54:38 hidalgo kernel: thinkpad_acpi: Standard ACPI backlight interface
> not available, thinkpad_acpi driver will take over control Aug  4 07:54:38
> hidalgo kernel: thinkpad_acpi: detected a 16-level brightness capable
> ThinkPad Aug  4 07:54:38 hidalgo kernel: input: ThinkPad Extra Buttons as
> /devices/virtual/input/input6 Aug  4 07:54:38 hidalgo kernel: usb 1-1: new
> full speed USB device using uhci_hcd and address 4 Aug  4 07:54:38 hidalgo
> kernel: usb 1-1: configuration #1 chosen from 1 choice
>
> But when in user mode, brightness keys doesn't do anything either.
> Echo'ing to /proc/acpi/ibm/brightness doesn't work, but echoing
> to /sys/class/backlight/thinkpad_screen/brightness works.
That /proc/acpi/ibm/brightness does not and
/sys/class/backlight/thinkpad_screen/brightness 
works is strange, this should be a bug?

I wonder whether /proc/acpi/ibm/brightness can be removed soon, people had
some time to make use of the generic interface.

What you describe very much reminds me when I tested on a ThinkPad with an IGD
device. I removed the check for IGD devices in favour of a working IGD driver.

Attached patch might help you (with acpi_backlight=vendor)?
You should be able to apply this on my latest patchset, but use this patch
instead of the thinkpad_acpi one.

> Cat'ing /proc/acpi/events still gives:
> video LCD0 00000086 00000000
> video LCD0 00000087 00000000
>
> At one time I get the 750ms delay, but I don't have the
> ibm/hotkey HKEY 00000080 00005010 at all.
Hmm, on my machine it was 1010 and 1011 key event values for up/down
brightness. If you do not see those, the patch might not work.

> In X, same thing, no brightness keys, but xbacklight does work in
> standard mode (“kernel”).
>
> (btw it seems my mails don't reach linux-acpi, I don't really know why
> and postmaster don't answer, maybe they didn't have my mail either).

I expect you have an IGD device and this should by default be served by
Matthew's/Hong's patches. Matthew said something about a needed peace
that came in in latest BIOSes? But I expect you already run on the latest?
If not, you should update.

        Thomas



diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index 85c3d2c..c9ed761 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -93,6 +93,11 @@ long acpi_is_video_device(struct acpi_device *device)
 	    ACPI_SUCCESS(acpi_get_handle(device->handle, "_SPD", &h_dummy)))
 		video_caps |= ACPI_VIDEO_DEVICE_POSTING;
 
+	if (ACPI_SUCCESS(acpi_get_handle(device->handle, "DRDY", &h_dummy))) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found IGD device\n"));
+		video_caps |= ACPI_VIDEO_IGD;
+	}
+
 	/* Only check for backlight functionality if one of the above hit. */
 	if (video_caps)
 		acpi_walk_namespace(ACPI_TYPE_DEVICE, device->handle,
diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index d3eb790..c68c6b1 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -240,6 +240,7 @@ static struct {
 	u32 light_status:1;
 	u32 bright_16levels:1;
 	u32 bright_acpimode:1;
+	u32 bright_igdmode:1;
 	u32 wan:1;
 	u32 fan_ctrl_status_undef:1;
 	u32 input_device_registered:1;
@@ -2353,6 +2354,9 @@ err_exit:
 	return (res < 0)? res : 1;
 }
 
+static struct backlight_device *ibm_backlight_device;
+static int brightness_update_status(struct backlight_device *bd);
+
 static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 {
 	u32 hkey;
@@ -2391,6 +2395,28 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
 		case 1:
 			/* 0x1000-0x1FFF: key presses */
 			scancode = hkey & 0xfff;
+			if (tp_features.bright_igdmode) {
+				/* ToDo:
+				 * Is there an already defined key?
+				 */
+				if (hkey == 0x1011) {
+					if (ibm_backlight_device->
+					    props.brightness > 0) {
+						ibm_backlight_device->
+							props.brightness--;
+					}
+				} else if (hkey == 0x1010) {
+					if (ibm_backlight_device->
+					    props.brightness <
+					    ibm_backlight_device->
+					    props.max_brightness) {
+						ibm_backlight_device->
+							props.brightness++;
+					}
+				}
+				brightness_update_status(ibm_backlight_device);
+			}
+
 			if (scancode > 0 && scancode < 0x21) {
 				scancode--;
 				if (!(hotkey_source_mask & (1 << scancode))) {
@@ -4771,7 +4797,6 @@ enum {
 	TP_EC_BACKLIGHT_MAPSW = 0x20,
 };
 
-static struct backlight_device *ibm_backlight_device;
 static int brightness_mode;
 static unsigned int brightness_enable = 2; /* 2 = auto, 0 = no, 1 = yes */
 
@@ -4910,7 +4935,7 @@ static struct backlight_ops ibm_backlight_data = {
 static int __init brightness_init(struct ibm_init_struct *iibm)
 {
 	int b;
-
+	long acpi_video_support;
 	vdbg_printk(TPACPI_DBG_INIT, "initializing brightness subdriver\n");
 
 	mutex_init(&brightness_mutex);
@@ -4922,17 +4947,43 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
 	 */
 	b = tpacpi_check_std_acpi_brightness_support();
 	if (b > 0) {
-		if (thinkpad_id.vendor == PCI_VENDOR_ID_LENOVO) {
-			printk(TPACPI_NOTICE
-			       "Lenovo BIOS switched to ACPI backlight "
-			       "control mode\n");
-		}
-		if (brightness_enable > 1) {
-			printk(TPACPI_NOTICE
-			       "standard ACPI backlight interface "
-			       "available, not loading native one...\n");
-			return 1;
-		}
+
+		if (acpi_video_backlight_support()) {
+			if (brightness_enable > 1) {
+				printk(TPACPI_NOTICE
+				       "standard ACPI backlight interface "
+				       "available, not loading native one.\n");
+				return 1;
+			} else if (brightness_enable == 1) {
+				printk(TPACPI_NOTICE
+				       "Backlight control force, even standard "
+				       "ACPI backlight interface available\n");
+			}
+		} else {
+			if (brightness_enable > 1) {
+				printk(TPACPI_NOTICE
+				       "Standard ACPI backlight interface "
+				       "available, but thinkpad_acpi driver "
+				       "will take over control\n");
+			}
+			/* We have an Integrated Graphics Device and BIOS
+			 * won't switch. By default the IGD driver should take
+			 * care about backlight and display output switching.
+			 * People can still run into this code path and let
+			 * thinkpad_acpi take over control by passing:
+			 * acpi_backlight=vendor or acpi_display_output=vendor
+			 * boot params.
+			 */
+			acpi_video_support = acpi_video_get_capabilities(NULL);
+			vdbg_printk(TPACPI_DBG_INIT, "XXXXXk\n");
+
+			if (acpi_video_support & ACPI_VIDEO_IGD) {
+				vdbg_printk(TPACPI_DBG_INIT, "IGD device"
+					    " detected - take over backlight"
+					    " switching\n");
+				tp_features.bright_igdmode = 1;
+			}
+ 		}
 	}
 
 	if (!brightness_enable) {
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index eb51726..cd012a3 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -214,6 +214,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_IGD					0x1000
 
 #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
 
--
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: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Yves-Alexis Perez]

On Mon, Aug 04, 2008 at 12:20:28PM +0200, Thomas Renninger wrote:
> > But when in user mode, brightness keys doesn't do anything either.
> > Echo'ing to /proc/acpi/ibm/brightness doesn't work, but echoing
> > to /sys/class/backlight/thinkpad_screen/brightness works.
> That /proc/acpi/ibm/brightness does not and
> /sys/class/backlight/thinkpad_screen/brightness 
> works is strange, this should be a bug?

I guess so, but I don't really know how to check.

> 
> I wonder whether /proc/acpi/ibm/brightness can be removed soon, people had
> some time to make use of the generic interface.

I'm not using it usually anyway.
> 
> What you describe very much reminds me when I tested on a ThinkPad with an IGD
> device. I removed the check for IGD devices in favour of a working IGD driver.
> 
> Attached patch might help you (with acpi_backlight=vendor)?
> You should be able to apply this on my latest patchset, but use this patch
> instead of the thinkpad_acpi one.

Ok, will try tonight. But I remember Henrique saying my thinkpad
shouldnt use thinkpad-acpi but rather video.ko for brightness
management, so I'm a bit puzzled.

> > Cat'ing /proc/acpi/events still gives:
> > video LCD0 00000086 00000000
> > video LCD0 00000087 00000000
> >
> > At one time I get the 750ms delay, but I don't have the
> > ibm/hotkey HKEY 00000080 00005010 at all.
> Hmm, on my machine it was 1010 and 1011 key event values for up/down
> brightness. If you do not see those, the patch might not work.

And I do remember that the key changed (but can't remember when). Maybe
that's due to a bios upgrade.

> > In X, same thing, no brightness keys, but xbacklight does work in
> > standard mode (“kernel”).
> >
> > (btw it seems my mails don't reach linux-acpi, I don't really know why
> > and postmaster don't answer, maybe they didn't have my mail either).
> 
> I expect you have an IGD device and this should by default be served by
> Matthew's/Hong's patches. Matthew said something about a needed peace
> that came in in latest BIOSes? But I expect you already run on the latest?

I run Bios 2.19 and EC 1.08 which are the latest available on
lenovo.com.

The kernel I'm testing on is a pure linux-acpi-2.6/test so i'm not sure
I have Matthew's patches.

Cheers,
-- 
Yves-Alexis
--
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: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Thomas Renninger]

On Monday 04 August 2008 12:39:30 Yves-Alexis Perez wrote:
> On Mon, Aug 04, 2008 at 12:20:28PM +0200, Thomas Renninger wrote:
> > > But when in user mode, brightness keys doesn't do anything either.
> > > Echo'ing to /proc/acpi/ibm/brightness doesn't work, but echoing
> > > to /sys/class/backlight/thinkpad_screen/brightness works.
> >
> > That /proc/acpi/ibm/brightness does not and
> > /sys/class/backlight/thinkpad_screen/brightness
> > works is strange, this should be a bug?
>
> I guess so, but I don't really know how to check.
>
> > I wonder whether /proc/acpi/ibm/brightness can be removed soon, people
> > had some time to make use of the generic interface.
>
> I'm not using it usually anyway.
>
> > What you describe very much reminds me when I tested on a ThinkPad with
> > an IGD device. I removed the check for IGD devices in favour of a working
> > IGD driver.
> >
> > Attached patch might help you (with acpi_backlight=vendor)?
> > You should be able to apply this on my latest patchset, but use this
> > patch instead of the thinkpad_acpi one.
>
> Ok, will try tonight. But I remember Henrique saying my thinkpad
> shouldnt use thinkpad-acpi but rather video.ko for brightness
> management, so I'm a bit puzzled.
>
> > > Cat'ing /proc/acpi/events still gives:
> > > video LCD0 00000086 00000000
> > > video LCD0 00000087 00000000
> > >
> > > At one time I get the 750ms delay, but I don't have the
> > > ibm/hotkey HKEY 00000080 00005010 at all.
> >
> > Hmm, on my machine it was 1010 and 1011 key event values for up/down
> > brightness. If you do not see those, the patch might not work.
>
> And I do remember that the key changed (but can't remember when). Maybe
> that's due to a bios upgrade.
>
> > > In X, same thing, no brightness keys, but xbacklight does work in
> > > standard mode (“kernel”).
> > >
> > > (btw it seems my mails don't reach linux-acpi, I don't really know why
> > > and postmaster don't answer, maybe they didn't have my mail either).
> >
> > I expect you have an IGD device and this should by default be served by
> > Matthew's/Hong's patches. Matthew said something about a needed peace
> > that came in in latest BIOSes? But I expect you already run on the
> > latest?
>
> I run Bios 2.19 and EC 1.08 which are the latest available on
> lenovo.com.
>
> The kernel I'm testing on is a pure linux-acpi-2.6/test so i'm not sure
> I have Matthew's patches.

And this probably is the problem.
They seem not to be in:
branch 'test' of 
git://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-acpi-2.6

I just tried to find them in the tree that I thought Matthew stated that they 
are queued their in the master branch:
git://git.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git
Without success, neither in the master nor in the -mm nor in the linux-next 
branch I could find anything with Matthew in CC or pointing to opregion.

The last post from Matthew on the dri list sending an update patch is from:
2008-05-29
This was before he found out that on ThinkPads a PWM method has to be called.

Something is fishy here.
Seems there are still problems? Maybe I should add the IGD check again, then 
it's not harmful if the two patchsets do not go in together...

      Thomas
--
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: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Matthew Garrett]

On Mon, Aug 04, 2008 at 01:19:42PM +0200, Thomas Renninger wrote:

> I just tried to find them in the tree that I thought Matthew stated that they 
> are queued their in the master branch:
> git://git.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git

It got dropped after the vblank IRQ rewrite code got merged. I'll have 
it remerged today.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Yves-Alexis Perez]

On Mon, Aug 04, 2008 at 02:24:50PM +0100, Matthew Garrett wrote:
> On Mon, Aug 04, 2008 at 01:19:42PM +0200, Thomas Renninger wrote:
> 
> > I just tried to find them in the tree that I thought Matthew stated that they 
> > are queued their in the master branch:
> > git://git.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git
> 
> It got dropped after the vblank IRQ rewrite code got merged. I'll have 
> it remerged today.

Hmhm so I should cherry-pick those patches (when merged) from drm-2.6 to
linux-acpi-2.6/test plus the previously attached patch from Thomas?

Cheers,
-- 
Yves-Alexis

Re: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Thomas Renninger]

On Monday 04 August 2008 15:30:17 Yves-Alexis Perez wrote:
> On Mon, Aug 04, 2008 at 02:24:50PM +0100, Matthew Garrett wrote:
> > On Mon, Aug 04, 2008 at 01:19:42PM +0200, Thomas Renninger wrote:
> > > I just tried to find them in the tree that I thought Matthew stated
> > > that they are queued their in the master branch:
> > > git://git.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git
> >
> > It got dropped after the vblank IRQ rewrite code got merged. I'll have
> > it remerged today.
>
> Hmhm so I should cherry-pick those patches (when merged) from drm-2.6 to
> linux-acpi-2.6/test plus the previously attached patch from Thomas?

Best just try Matthews, together with mine which went into Andi's test tree 
(version 4).
The additional patch I sent today is only needed if Matthew's does not work 
for you and you want to try acpi_backlight=vendor on an IGD ThinkPad device.

But instead of trying the additional one, we should find out why Matthew's 
things do not work (hopefully they do work). You already correctly stated: 
video.ko is the way to go for your machine.
When say in -rc5 you still have problems, the additional one can be 
tested/added, so that all the rest does not need to be reverted and we do not 
get regressions in 2.6.27.

       Thomas

Re: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Yves-Alexis Perez]

> > Hmhm so I should cherry-pick those patches (when merged) from drm-2.6 to
> > linux-acpi-2.6/test plus the previously attached patch from Thomas?
> 
> Best just try Matthews, together with mine which went into Andi's test tree 
> (version 4).
> The additional patch I sent today is only needed if Matthew's does not work 
> for you and you want to try acpi_backlight=vendor on an IGD ThinkPad device.

Ok so I'll cherry pick Matthew's patches, test and report back tonight.

> When say in -rc5 you still have problems, the additional one can be 
> tested/added, so that all the rest does not need to be reverted and we do not 
> get regressions in 2.6.27.


Hmh but linux-acpi-2.6/test is meant to be merged mainline for 2.6.27?

Cheers,
-- 
Yves-Alexis

Re: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Henrique de Moraes Holschuh]

On Mon, 04 Aug 2008, Thomas Renninger wrote:
> On Monday 04 August 2008 08:09:52 Yves-Alexis Perez wrote:
> > On lun, 2008-08-04 at 09:16 +0800, Zhang Rui wrote:
> > > I think you're using the laptop that this patch does break.
> > > Please try acpi_backlight=vendor.
> >
> > When booting to single user with acpi_backlight=vendor, thinkpad-acpi
> > takes over backlight:
> >
> > Aug  4 07:54:38 hidalgo kernel:
> > thinkpad_acpi: Disabling thinkpad-acpi brightness events by default...

See that one?

> > But when in user mode, brightness keys doesn't do anything either.
> > Echo'ing to /proc/acpi/ibm/brightness doesn't work, but echoing
> > to /sys/class/backlight/thinkpad_screen/brightness works.
> That /proc/acpi/ibm/brightness does not and
> /sys/class/backlight/thinkpad_screen/brightness 
> works is strange, this should be a bug?

No.  It is just that the driver expects ACPI video to be sending the events,
and it isn't, so you need to change some code in hotkey_init to stop
disabling brightness events in the (ACPI video support in DSDT, ACPI video
disabled and thinkpad-acpi is to handle the backlight) case.

> I wonder whether /proc/acpi/ibm/brightness can be removed soon, people had
> some time to make use of the generic interface.

It will go when everything else in /proc/acpi/ibm goes.  Which will happen
when /proc/acpi goes, or when it is intolerable to keep supporting it for
some good reason.

> Attached patch might help you (with acpi_backlight=vendor)?
> You should be able to apply this on my latest patchset, but use this patch
> instead of the thinkpad_acpi one.

HEAVY NAK on the thinkpad-acpi hunks in this one.  It is good for testing,
but I don't want any automated handling of the brightness hotkeys in
thinkpad-acpi unless we absolutely HAVE to emulate the older non-Vista
firmware for some weird reason.  More on this at the end of this post.

Please check if updating hotkey_init doesn't solve the issue.  I am sorry I
didn't recall that detail to remind you about it when I looked your patches
over.

> > Cat'ing /proc/acpi/events still gives:
> > video LCD0 00000086 00000000
> > video LCD0 00000087 00000000
> >
> > At one time I get the 750ms delay, but I don't have the
> > ibm/hotkey HKEY 00000080 00005010 at all.

This is caused by EC+ACPI DSDT access.  Switch thinkpad-acpi brightness_mode
to use only ACPI DSDT ("CMOS" mode), and it should go away.  This should be
the default on all Lenovo, though.

Of course, this also mean you have nothing in userspace messing with the
RTC NVRAM's brightness bits, because if you do, you will still get bugs.

> @@ -2391,6 +2395,28 @@ static void hotkey_notify(struct ibm_struct *ibm, u32 event)
>  		case 1:
>  			/* 0x1000-0x1FFF: key presses */
>  			scancode = hkey & 0xfff;
> +			if (tp_features.bright_igdmode) {
> +				/* ToDo:
> +				 * Is there an already defined key?
> +				 */
> +				if (hkey == 0x1011) {
> +					if (ibm_backlight_device->
> +					    props.brightness > 0) {
> +						ibm_backlight_device->
> +							props.brightness--;
> +					}
> +				} else if (hkey == 0x1010) {
> +					if (ibm_backlight_device->
> +					    props.brightness <
> +					    ibm_backlight_device->
> +					    props.max_brightness) {
> +						ibm_backlight_device->
> +							props.brightness++;
> +					}
> +				}
> +				brightness_update_status(ibm_backlight_device);
> +			}
> +
>  			if (scancode > 0 && scancode < 0x21) {
>  				scancode--;
>  				if (!(hotkey_source_mask & (1 << scancode))) {

As I said, the above is NAK'd, except for the call to
brightness_update_status(ibm_backlight_device), which you should keep if it
is really needed.

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

Re: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Yves-Alexis Perez]

[-- Attachment #1: Type: text/plain, Size: 2881 bytes --]

On lun, 2008-08-04 at 21:01 -0300, Henrique de Moraes Holschuh wrote:
> On Mon, 04 Aug 2008, Thomas Renninger wrote:
> > On Monday 04 August 2008 08:09:52 Yves-Alexis Perez wrote:
> > > When booting to single user with acpi_backlight=vendor, thinkpad-acpi
> > > takes over backlight:
> > >
> > > Aug  4 07:54:38 hidalgo kernel:
> > > thinkpad_acpi: Disabling thinkpad-acpi brightness events by default...
> 
> See that one?

Hmhm yeah but then I have:
Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: Standard ACPI backlight interface not available, thinkpad_acpi driver will take over control


> > > But when in user mode, brightness keys doesn't do anything either.
> > > Echo'ing to /proc/acpi/ibm/brightness doesn't work, but echoing
> > > to /sys/class/backlight/thinkpad_screen/brightness works.
> > That /proc/acpi/ibm/brightness does not and
> > /sys/class/backlight/thinkpad_screen/brightness 
> > works is strange, this should be a bug?
> 
> No.  It is just that the driver expects ACPI video to be sending the events,
> and it isn't, so you need to change some code in hotkey_init to stop
> disabling brightness events in the (ACPI video support in DSDT, ACPI video
> disabled and thinkpad-acpi is to handle the backlight) case.

Hmhm ok, so the thinkpad-acpi related patch miss some case handling
anyway?


> > Attached patch might help you (with acpi_backlight=vendor)?
> > You should be able to apply this on my latest patchset, but use this patch
> > instead of the thinkpad_acpi one.
> 
> HEAVY NAK on the thinkpad-acpi hunks in this one.  It is good for testing,
> but I don't want any automated handling of the brightness hotkeys in
> thinkpad-acpi unless we absolutely HAVE to emulate the older non-Vista
> firmware for some weird reason.  More on this at the end of this post.

Anyway I'd prefer going the “normal” way and use video.ko.

> 
> Please check if updating hotkey_init doesn't solve the issue.  I am sorry I
> didn't recall that detail to remind you about it when I looked your patches
> over.

How should I update hotkey_init? Check for acpi_backlight=vendor and not
disable brightness event?

> > > Cat'ing /proc/acpi/events still gives:
> > > video LCD0 00000086 00000000
> > > video LCD0 00000087 00000000
> > >
> > > At one time I get the 750ms delay, but I don't have the
> > > ibm/hotkey HKEY 00000080 00005010 at all.
> 
> This is caused by EC+ACPI DSDT access.  Switch thinkpad-acpi brightness_mode
> to use only ACPI DSDT ("CMOS" mode), and it should go away.  This should be
> the default on all Lenovo, though.

Hmmh, that's weird because I don't think I'm messing with the
brightness_mode options. But I can force it to use only CMOS mode and
test.

Anyway, I wasn't yet able to test Matthew's patches, they're not yet in
drm-2.6/master.

Cheers,
-- 
Yves-Alexis

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Thomas Renninger]

On Tuesday 05 August 2008 07:55:53 Yves-Alexis Perez wrote:
...
> Anyway, I wasn't yet able to test Matthew's patches, they're not yet in
> drm-2.6/master.
This should definitely be the next step.
If this fails we should still try to dig there why it is not working.

I agree, that only if we heavily get stuck there it would make sense
to look at thinkpad_acpi to enable/workaround backlight switching with
IGD devices.

     Thomas

Re: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Yves-Alexis Perez]

On Tue, Aug 05, 2008 at 11:13:42AM +0200, Thomas Renninger wrote:
> On Tuesday 05 August 2008 07:55:53 Yves-Alexis Perez wrote:
> ...
> > Anyway, I wasn't yet able to test Matthew's patches, they're not yet in
> > drm-2.6/master.
> This should definitely be the next step.
> If this fails we should still try to dig there why it is not working.

How can I test (and maybe debug) your patches then?

> 
> I agree, that only if we heavily get stuck there it would make sense
> to look at thinkpad_acpi to enable/workaround backlight switching with
> IGD devices.

Hmhm, I assume that, under vista, IGD and Nvidia ones are managed the
same (as far as I understood it, and using acpi backlight management),
so I guess it would be a "last chance" solution?

Cheers,
-- 
Yves-Alexis

Re: Check for ACPI backlight support otherwise use vendor ACPIdrivers - version 4

[Henrique de Moraes Holschuh]

On Tue, 05 Aug 2008, Yves-Alexis Perez wrote:
> On lun, 2008-08-04 at 21:01 -0300, Henrique de Moraes Holschuh wrote:
> > On Mon, 04 Aug 2008, Thomas Renninger wrote:
> > > On Monday 04 August 2008 08:09:52 Yves-Alexis Perez wrote:
> > > > When booting to single user with acpi_backlight=vendor, thinkpad-acpi
> > > > takes over backlight:
> > > >
> > > > Aug  4 07:54:38 hidalgo kernel:
> > > > thinkpad_acpi: Disabling thinkpad-acpi brightness events by default...
> > 
> > See that one?
> 
> Hmhm yeah but then I have:
> Aug  4 07:54:38 hidalgo kernel: thinkpad_acpi: Standard ACPI backlight interface not available, thinkpad_acpi driver will take over control

Two different subdrivers.  It would be better if I added subdriver
identification to all messages, I think.  hotkey is telling you it is not
issue brightness events.  brightness is telling you it is going to take over
control of brightness.

> > > > But when in user mode, brightness keys doesn't do anything either.
> > > > Echo'ing to /proc/acpi/ibm/brightness doesn't work, but echoing
> > > > to /sys/class/backlight/thinkpad_screen/brightness works.
> > > That /proc/acpi/ibm/brightness does not and
> > > /sys/class/backlight/thinkpad_screen/brightness 
> > > works is strange, this should be a bug?
> > 
> > No.  It is just that the driver expects ACPI video to be sending the events,
> > and it isn't, so you need to change some code in hotkey_init to stop
> > disabling brightness events in the (ACPI video support in DSDT, ACPI video
> > disabled and thinkpad-acpi is to handle the backlight) case.
> 
> Hmhm ok, so the thinkpad-acpi related patch miss some case handling
> anyway?

Yes.

> > Please check if updating hotkey_init doesn't solve the issue.  I am sorry I
> > didn't recall that detail to remind you about it when I looked your patches
> > over.
> 
> How should I update hotkey_init? Check for acpi_backlight=vendor and not
> disable brightness event?

Yes.  But Thomas will need to update his patch to do this, I think.  Unless
ACPI video is left active enough to pass on the ACPI video change events
(but I have no idea if the DSDT will still issue those events, etc).

Thomas?

> > > > Cat'ing /proc/acpi/events still gives:
> > > > video LCD0 00000086 00000000
> > > > video LCD0 00000087 00000000
> > > >
> > > > At one time I get the 750ms delay, but I don't have the
> > > > ibm/hotkey HKEY 00000080 00005010 at all.
> > 
> > This is caused by EC+ACPI DSDT access.  Switch thinkpad-acpi brightness_mode
> > to use only ACPI DSDT ("CMOS" mode), and it should go away.  This should be
> > the default on all Lenovo, though.
> 
> Hmmh, that's weird because I don't think I'm messing with the
> brightness_mode options. But I can force it to use only CMOS mode and
> test.

Well, please check it anyway.  I doubt it is that, but it can't hurt to
force brightness_mode to CMOS only.

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