[PATCH] ACPI: disable the ACPI backlight control on laptops with buggy _BCL methods

[PATCH] ACPI: disable the ACPI backlight control on laptops with buggy _BCL methods

[Zhang Rui]

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

From: Zhang Rui <rui.zhang@intel.com>
Subject: disable the ACPI backlight control on laptops with buggy _BCL methods

Some users used to control the backlight via the platform interface.
But commit c3d6de698c84efdbdd3781b7058bcc339ab43da8 disables the platform
backlight control if ACPI video backlight control is available.

This breaks the laptops with buggy _BCL/_BCM/_BQC methods. i.e. only ACPI
backlight I/F are available on these laptops but they don't work.

With this patch applied, the ACPI backlight control are disabled instead
on these laptops.

http://bugzilla.kernel.org/show_bug.cgi?id=12037
http://bugzilla.kernel.org/show_bug.cgi?id=12235
http://bugzilla.kernel.org/show_bug.cgi?id=12249
http://bugzilla.kernel.org/show_bug.cgi?id=12302
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/311716
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/301524

CC: Thomas Renninger <trenn@suse.de>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/video_detect.c |   83 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 77 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/acpi/video_detect.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video_detect.c
+++ linux-2.6/drivers/acpi/video_detect.c
@@ -44,17 +44,88 @@ static long acpi_video_support;
 static bool acpi_video_caps_checked;
 
 static acpi_status
+acpi_backlight_validate_bcl(acpi_handle handle)
+{
+	union acpi_object *obj, *obj2;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	int level_ac = 0, level_battery = 0;
+	int i, count;
+	int *levels;
+	acpi_status status;
+
+
+	status = acpi_evaluate_object(handle, "_BCL", NULL, &buffer);
+	if (!ACPI_SUCCESS(status))
+		return status;
+	obj = (union acpi_object *)buffer.pointer;
+	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
+		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
+		status = AE_BAD_DATA;
+		goto out;
+	}
+
+	if (obj->package.count < 2) {
+		status = AE_BAD_DATA;
+		goto out;
+	}
+
+        levels = kzalloc(obj->package.count * sizeof(*levels), GFP_KERNEL);
+	if (!levels) {
+		status = AE_NO_MEMORY;
+		goto out;
+	}
+
+	for (i = 0, count = 0; i < obj->package.count; i++) {
+		obj2 = (union acpi_object *)&obj->package.elements[i];
+		if (obj2->type != ACPI_TYPE_INTEGER) {
+			printk(KERN_ERR PREFIX "Invalid data\n");
+			continue;
+		}
+		levels[i] = (u32) obj2->integer.value;
+		count ++;
+        }
+
+        if (count < 2) {
+		status = AE_BAD_DATA;
+		goto out_free_levels;
+	}
+
+	for (i = 2, level_ac = levels[0], level_battery = levels[1]; i < count; i ++) {
+		if (levels[0] == levels[i])
+			level_ac = 1;
+		if (levels[1] == levels[i])
+			level_battery = 1;
+	}
+
+	if (!level_ac || !level_battery) {
+		printk(KERN_ERR PREFIX "Invalid _BCL package\n");
+		status = AE_BAD_DATA;
+		goto out_free_levels;
+	}
+
+        ACPI_DEBUG_PRINT((ACPI_DB_INFO, "found %d brightness levels\n", count));
+
+out_free_levels:
+	kfree(levels);
+out:
+        kfree(obj);
+        return status;
+}
+
+static acpi_status
 acpi_backlight_cap_match(acpi_handle handle, u32 level, void *context,
 			  void **retyurn_value)
 {
 	long *cap = context;
-	acpi_handle h_dummy;
+	acpi_handle h_dummy1, h_dummy2;
 
-	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;
+	if (ACPI_SUCCESS(acpi_get_handle(handle, "_BCM", &h_dummy1)) &&
+	    ACPI_SUCCESS(acpi_get_handle(handle, "_BCL", &h_dummy2))) {
+		if (ACPI_SUCCESS(acpi_backlight_validate_bcl(h_dummy2))) {
+			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;
 	}


[-- Attachment #2: patch-video-check-_BCL --]
[-- Type: message/rfc822, Size: 4036 bytes --]

From: Zhang Rui <rui.zhang@intel.com>
Subject: don't use buggy _BCL/_BCM/_BQC for backlight control
Date: Thu, 08 Jan 2009 10:56:50 +0800
Message-ID: <1231383410.20746.42.camel@rzhang-dt>

Some users used to control the backlight via the platform interface.
But commit c3d6de698c84efdbdd3781b7058bcc339ab43da8 disables the platform
backlight control if ACPI video backlight control is available.

This breaks the laptops with buggy _BCL/_BCM/_BQC methods. i.e. only ACPI
backlight I/F are available on these laptops but they don't work.

With this patch applied, the ACPI backlight control are disabled instead
on these laptops.

http://bugzilla.kernel.org/show_bug.cgi?id=12037
http://bugzilla.kernel.org/show_bug.cgi?id=12235
http://bugzilla.kernel.org/show_bug.cgi?id=12249
http://bugzilla.kernel.org/show_bug.cgi?id=12302
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/311716
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/301524

CC: Thomas Renninger <trenn@suse.de>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/video_detect.c |   83 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 77 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/acpi/video_detect.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video_detect.c
+++ linux-2.6/drivers/acpi/video_detect.c
@@ -44,17 +44,88 @@ static long acpi_video_support;
 static bool acpi_video_caps_checked;
 
 static acpi_status
+acpi_backlight_validate_bcl(acpi_handle handle)
+{
+	union acpi_object *obj, *obj2;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	int level_ac = 0, level_battery = 0;
+	int i, count;
+	int *levels;
+	acpi_status status;
+
+
+	status = acpi_evaluate_object(handle, "_BCL", NULL, &buffer);
+	if (!ACPI_SUCCESS(status))
+		return status;
+	obj = (union acpi_object *)buffer.pointer;
+	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
+		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
+		status = AE_BAD_DATA;
+		goto out;
+	}
+
+	if (obj->package.count < 2) {
+		status = AE_BAD_DATA;
+		goto out;
+	}
+
+        levels = kzalloc(obj->package.count * sizeof(*levels), GFP_KERNEL);
+	if (!levels) {
+		status = AE_NO_MEMORY;
+		goto out;
+	}
+
+	for (i = 0, count = 0; i < obj->package.count; i++) {
+		obj2 = (union acpi_object *)&obj->package.elements[i];
+		if (obj2->type != ACPI_TYPE_INTEGER) {
+			printk(KERN_ERR PREFIX "Invalid data\n");
+			continue;
+		}
+		levels[i] = (u32) obj2->integer.value;
+		count ++;
+        }
+
+        if (count < 2) {
+		status = AE_BAD_DATA;
+		goto out_free_levels;
+	}
+
+	for (i = 2, level_ac = levels[0], level_battery = levels[1]; i < count; i ++) {
+		if (levels[0] == levels[i])
+			level_ac = 1;
+		if (levels[1] == levels[i])
+			level_battery = 1;
+	}
+
+	if (!level_ac || !level_battery) {
+		printk(KERN_ERR PREFIX "Invalid _BCL package\n");
+		status = AE_BAD_DATA;
+		goto out_free_levels;
+	}
+
+        ACPI_DEBUG_PRINT((ACPI_DB_INFO, "found %d brightness levels\n", count));
+
+out_free_levels:
+	kfree(levels);
+out:
+        kfree(obj);
+        return status;
+}
+
+static acpi_status
 acpi_backlight_cap_match(acpi_handle handle, u32 level, void *context,
 			  void **retyurn_value)
 {
 	long *cap = context;
-	acpi_handle h_dummy;
+	acpi_handle h_dummy1, h_dummy2;
 
-	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;
+	if (ACPI_SUCCESS(acpi_get_handle(handle, "_BCM", &h_dummy1)) &&
+	    ACPI_SUCCESS(acpi_get_handle(handle, "_BCL", &h_dummy2))) {
+		if (ACPI_SUCCESS(acpi_backlight_validate_bcl(h_dummy2))) {
+			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;
 	}

Re: [PATCH] ACPI: disable the ACPI backlight control on laptops with buggy _BCL methods

[Zhang Rui]

On Thu, 2009-01-08 at 10:58 +0800, Zhang, Rui wrote:
> From: Zhang Rui <rui.zhang@intel.com>
> Subject: disable the ACPI backlight control on laptops with buggy _BCL methods
> 
> Some users used to control the backlight via the platform interface.
> But commit c3d6de698c84efdbdd3781b7058bcc339ab43da8 disables the platform
> backlight control if ACPI video backlight control is available.
> 
> This breaks the laptops with buggy _BCL/_BCM/_BQC methods. i.e. only ACPI
> backlight I/F are available on these laptops but they don't work.
> 
> With this patch applied, the ACPI backlight control are disabled instead
> on these laptops.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=12037
> http://bugzilla.kernel.org/show_bug.cgi?id=12235
> http://bugzilla.kernel.org/show_bug.cgi?id=12249
> http://bugzilla.kernel.org/show_bug.cgi?id=12302
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/311716
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/301524
> 
> CC: Thomas Renninger <trenn@suse.de>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
tested-by: Andy Whitcroft <apw@canonical.com>

> ---
>  drivers/acpi/video_detect.c |   83 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 77 insertions(+), 6 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/video_detect.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/video_detect.c
> +++ linux-2.6/drivers/acpi/video_detect.c
> @@ -44,17 +44,88 @@ static long acpi_video_support;
>  static bool acpi_video_caps_checked;
>  
>  static acpi_status
> +acpi_backlight_validate_bcl(acpi_handle handle)
> +{
> +	union acpi_object *obj, *obj2;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	int level_ac = 0, level_battery = 0;
> +	int i, count;
> +	int *levels;
> +	acpi_status status;
> +
> +
> +	status = acpi_evaluate_object(handle, "_BCL", NULL, &buffer);
> +	if (!ACPI_SUCCESS(status))
> +		return status;
> +	obj = (union acpi_object *)buffer.pointer;
> +	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
> +		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
> +		status = AE_BAD_DATA;
> +		goto out;
> +	}
> +
> +	if (obj->package.count < 2) {
> +		status = AE_BAD_DATA;
> +		goto out;
> +	}
> +
> +        levels = kzalloc(obj->package.count * sizeof(*levels), GFP_KERNEL);
> +	if (!levels) {
> +		status = AE_NO_MEMORY;
> +		goto out;
> +	}
> +
> +	for (i = 0, count = 0; i < obj->package.count; i++) {
> +		obj2 = (union acpi_object *)&obj->package.elements[i];
> +		if (obj2->type != ACPI_TYPE_INTEGER) {
> +			printk(KERN_ERR PREFIX "Invalid data\n");
> +			continue;
> +		}
> +		levels[i] = (u32) obj2->integer.value;
> +		count ++;
> +        }
> +
> +        if (count < 2) {
> +		status = AE_BAD_DATA;
> +		goto out_free_levels;
> +	}
> +
> +	for (i = 2, level_ac = levels[0], level_battery = levels[1]; i < count; i ++) {
> +		if (levels[0] == levels[i])
> +			level_ac = 1;
> +		if (levels[1] == levels[i])
> +			level_battery = 1;
> +	}
> +
> +	if (!level_ac || !level_battery) {
> +		printk(KERN_ERR PREFIX "Invalid _BCL package\n");
> +		status = AE_BAD_DATA;
> +		goto out_free_levels;
> +	}
> +
> +        ACPI_DEBUG_PRINT((ACPI_DB_INFO, "found %d brightness levels\n", count));
> +
> +out_free_levels:
> +	kfree(levels);
> +out:
> +        kfree(obj);
> +        return status;
> +}
> +
> +static acpi_status
>  acpi_backlight_cap_match(acpi_handle handle, u32 level, void *context,
>  			  void **retyurn_value)
>  {
>  	long *cap = context;
> -	acpi_handle h_dummy;
> +	acpi_handle h_dummy1, h_dummy2;
>  
> -	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;
> +	if (ACPI_SUCCESS(acpi_get_handle(handle, "_BCM", &h_dummy1)) &&
> +	    ACPI_SUCCESS(acpi_get_handle(handle, "_BCL", &h_dummy2))) {
> +		if (ACPI_SUCCESS(acpi_backlight_validate_bcl(h_dummy2))) {
> +			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;
>  	}
> 
> email message attachment
> > -------- Forwarded Message --------
> > From: Zhang, Rui <rui.zhang@intel.com>
> > Subject: don't use buggy _BCL/_BCM/_BQC for backlight control
> > Date: Thu, 8 Jan 2009 10:56:50 +0800
> > 
> > Some users used to control the backlight via the platform interface.
> > But commit c3d6de698c84efdbdd3781b7058bcc339ab43da8 disables the platform
> > backlight control if ACPI video backlight control is available.
> > 
> > This breaks the laptops with buggy _BCL/_BCM/_BQC methods. i.e. only ACPI
> > backlight I/F are available on these laptops but they don't work.
> > 
> > With this patch applied, the ACPI backlight control are disabled instead
> > on these laptops.
> > 
> > http://bugzilla.kernel.org/show_bug.cgi?id=12037
> > http://bugzilla.kernel.org/show_bug.cgi?id=12235
> > http://bugzilla.kernel.org/show_bug.cgi?id=12249
> > http://bugzilla.kernel.org/show_bug.cgi?id=12302
> > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/311716
> > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/301524
> > 
> > CC: Thomas Renninger <trenn@suse.de>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/acpi/video_detect.c |   83 ++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 77 insertions(+), 6 deletions(-)
> > 
> > Index: linux-2.6/drivers/acpi/video_detect.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/video_detect.c
> > +++ linux-2.6/drivers/acpi/video_detect.c
> > @@ -44,17 +44,88 @@ static long acpi_video_support;
> >  static bool acpi_video_caps_checked;
> >  
> >  static acpi_status
> > +acpi_backlight_validate_bcl(acpi_handle handle)
> > +{
> > +	union acpi_object *obj, *obj2;
> > +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +	int level_ac = 0, level_battery = 0;
> > +	int i, count;
> > +	int *levels;
> > +	acpi_status status;
> > +
> > +
> > +	status = acpi_evaluate_object(handle, "_BCL", NULL, &buffer);
> > +	if (!ACPI_SUCCESS(status))
> > +		return status;
> > +	obj = (union acpi_object *)buffer.pointer;
> > +	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
> > +		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
> > +		status = AE_BAD_DATA;
> > +		goto out;
> > +	}
> > +
> > +	if (obj->package.count < 2) {
> > +		status = AE_BAD_DATA;
> > +		goto out;
> > +	}
> > +
> > +        levels = kzalloc(obj->package.count * sizeof(*levels), GFP_KERNEL);
> > +	if (!levels) {
> > +		status = AE_NO_MEMORY;
> > +		goto out;
> > +	}
> > +
> > +	for (i = 0, count = 0; i < obj->package.count; i++) {
> > +		obj2 = (union acpi_object *)&obj->package.elements[i];
> > +		if (obj2->type != ACPI_TYPE_INTEGER) {
> > +			printk(KERN_ERR PREFIX "Invalid data\n");
> > +			continue;
> > +		}
> > +		levels[i] = (u32) obj2->integer.value;
> > +		count ++;
> > +        }
> > +
> > +        if (count < 2) {
> > +		status = AE_BAD_DATA;
> > +		goto out_free_levels;
> > +	}
> > +
> > +	for (i = 2, level_ac = levels[0], level_battery = levels[1]; i < count; i ++) {
> > +		if (levels[0] == levels[i])
> > +			level_ac = 1;
> > +		if (levels[1] == levels[i])
> > +			level_battery = 1;
> > +	}
> > +
> > +	if (!level_ac || !level_battery) {
> > +		printk(KERN_ERR PREFIX "Invalid _BCL package\n");
> > +		status = AE_BAD_DATA;
> > +		goto out_free_levels;
> > +	}
> > +
> > +        ACPI_DEBUG_PRINT((ACPI_DB_INFO, "found %d brightness levels\n", count));
> > +
> > +out_free_levels:
> > +	kfree(levels);
> > +out:
> > +        kfree(obj);
> > +        return status;
> > +}
> > +
> > +static acpi_status
> >  acpi_backlight_cap_match(acpi_handle handle, u32 level, void *context,
> >  			  void **retyurn_value)
> >  {
> >  	long *cap = context;
> > -	acpi_handle h_dummy;
> > +	acpi_handle h_dummy1, h_dummy2;
> >  
> > -	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;
> > +	if (ACPI_SUCCESS(acpi_get_handle(handle, "_BCM", &h_dummy1)) &&
> > +	    ACPI_SUCCESS(acpi_get_handle(handle, "_BCL", &h_dummy2))) {
> > +		if (ACPI_SUCCESS(acpi_backlight_validate_bcl(h_dummy2))) {
> > +			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;
> >  	}

Re: [PATCH] ACPI: disable the ACPI backlight control on laptops with buggy _BCL methods

[Matthew Garrett]

On Thu, Jan 08, 2009 at 10:58:16AM +0800, Zhang Rui wrote:

> Some users used to control the backlight via the platform interface.
> But commit c3d6de698c84efdbdd3781b7058bcc339ab43da8 disables the platform
> backlight control if ACPI video backlight control is available.
> 
> This breaks the laptops with buggy _BCL/_BCM/_BQC methods. i.e. only ACPI
> backlight I/F are available on these laptops but they don't work.
> 
> With this patch applied, the ACPI backlight control are disabled instead
> on these laptops.

Why? These all seem to behave consistently, so I can't see any problem 
with us simply adding support for this varient. Disabling the ACPI 
control when we have no idea whether there's a platform driver available 
for the machine doesn't seem like the right fix.

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

Re: [PATCH] ACPI: disable the ACPI backlight control on laptops with buggy _BCL methods

[Zhang Rui]

On Thu, 2009-01-08 at 11:32 +0800, Matthew Garrett wrote:
> On Thu, Jan 08, 2009 at 10:58:16AM +0800, Zhang Rui wrote:
> 
> > Some users used to control the backlight via the platform interface.
> > But commit c3d6de698c84efdbdd3781b7058bcc339ab43da8 disables the platform
> > backlight control if ACPI video backlight control is available.
> > 
> > This breaks the laptops with buggy _BCL/_BCM/_BQC methods. i.e. only ACPI
> > backlight I/F are available on these laptops but they don't work.
> > 
> > With this patch applied, the ACPI backlight control are disabled instead
> > on these laptops.
> 
> Why? These all seem to behave consistently, so I can't see any problem 
> with us simply adding support for this varient.
the problem is that the ACPI backlight I/F and the platform I/F coexists
before commit c3d6de698c84efdbdd3781b7058bcc339ab43da8 applied, and the
platform I/F is preferred.

>  Disabling the ACPI 
> control when we have no idea whether there's a platform driver available 
> for the machine doesn't seem like the right fix.
> 
But once we enable the ACPI backlight control, the platform backlight
control is not available any more.

In these bugs, the _BCL/_BCM/_BQC method is really not well implemented.

in bug 12249 and 12037,
1. the first two elements in _BCL are NOT the backlight levels when the
platform is on AC or battery.
2. _BQC returns the index of the current brightness in _BCL package
rather than the value, which surely breaks the ACPI video driver.

in bug 12302
1. the first two elements in _BCL are NOT the backlight levels when the
platform is on AC or battery.
2. every elements in the _BCL package is not a percentage of the maximum
brightness, which is also a violation of ACPI spec.

yes, we can make the ACPI backlight control work by using customized
DSDT. But that's not the fix neither. I don't have any better ideas than
disable them.

thanks,
rui

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

Re: [PATCH] ACPI: disable the ACPI backlight control on laptops with buggy _BCL methods

[Matthew Garrett]

On Thu, Jan 08, 2009 at 02:32:01PM +0800, Zhang Rui wrote:
> > Why? These all seem to behave consistently, so I can't see any problem 
> > with us simply adding support for this varient.
> the problem is that the ACPI backlight I/F and the platform I/F coexists
> before commit c3d6de698c84efdbdd3781b7058bcc339ab43da8 applied, and the
> platform I/F is preferred.

For the machines you've looked at - we don't know if that's true for all 
hardware that behaves this way.

> in bug 12249 and 12037,
> 1. the first two elements in _BCL are NOT the backlight levels when the
> platform is on AC or battery.

We can detect that easily and adjust behaviour accordingly.

> 2. _BQC returns the index of the current brightness in _BCL package
> rather than the value, which surely breaks the ACPI video driver.

And again, we can handle that.

> 2. every elements in the _BCL package is not a percentage of the maximum
> brightness, which is also a violation of ACPI spec.

This seems pretty irrelevant - nothing in the code depends on these 
being percentages.

> yes, we can make the ACPI backlight control work by using customized
> DSDT. But that's not the fix neither. I don't have any better ideas than
> disable them.

Just fix up video.c to handle these cases.

-- 
Matthew Garrett | mjg59@srcf.ucam.org
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI: disable the ACPI backlight control on laptops with buggy _BCL methods

[Zhang Rui]

On Thu, 2009-01-08 at 20:43 +0800, Matthew Garrett wrote:
> On Thu, Jan 08, 2009 at 02:32:01PM +0800, Zhang Rui wrote:
> > > Why? These all seem to behave consistently, so I can't see any problem 
> > > with us simply adding support for this varient.
> > the problem is that the ACPI backlight I/F and the platform I/F coexists
> > before commit c3d6de698c84efdbdd3781b7058bcc339ab43da8 applied, and the
> > platform I/F is preferred.
> 
> For the machines you've looked at - we don't know if that's true for all 
> hardware that behaves this way.
> 
> > in bug 12249 and 12037,
> > 1. the first two elements in _BCL are NOT the backlight levels when the
> > platform is on AC or battery.
> 
> We can detect that easily and adjust behaviour accordingly.
yes.
we can fake two elements when evaluating _BCL package.
> 
> > 2. _BQC returns the index of the current brightness in _BCL package
> > rather than the value, which surely breaks the ACPI video driver.
> 
> And again, we can handle that.
No, we can not handle this one.
For example,
                    Name (PCTG, Package (0x10)
                    {
                        0x64,
                        0x5A,
                        0x55,
                        0x50,
                        0x4B,
                        0x46,
                        0x41,
                        0x3C,
                        0x37,
                        0x32,
                        0x2D,
                        0x28,
                        0x23,
                        0x1E,
                        0x19,
                        0x14
                    })

_BQC returns 3 when the actual backlight level is 0x50.
We only know that the value returned by _BQC is invalid but we never
know the value is an index until we see the _AML code.

thanks,
rui
> 
> > 2. every elements in the _BCL package is not a percentage of the maximum
> > brightness, which is also a violation of ACPI spec.
> 
> This seems pretty irrelevant - nothing in the code depends on these 
> being percentages.
> 
> > yes, we can make the ACPI backlight control work by using customized
> > DSDT. But that's not the fix neither. I don't have any better ideas than
> > disable them.
> 
> Just fix up video.c to handle these cases.
> 

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

Re: [PATCH] ACPI: disable the ACPI backlight control on laptops with buggy _BCL methods

[Matthew Garrett]

On Fri, Jan 09, 2009 at 09:13:38AM +0800, Zhang Rui wrote:

> 
> _BQC returns 3 when the actual backlight level is 0x50.
> We only know that the value returned by _BQC is invalid but we never
> know the value is an index until we see the _AML code.

If the value returned is less than the smallest value in the _BLC block, 
then it's an index. This heuristic will never break a standards 
compliant system, but (to the best of my knowledge) will work with any 
of these systems that we're currently aware of.

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

Re: [PATCH] ACPI: disable the ACPI backlight control on laptops with buggy _BCL methods

[Zhang Rui]

On Fri, 2009-01-09 at 09:18 +0800, Matthew Garrett wrote:
> On Fri, Jan 09, 2009 at 09:13:38AM +0800, Zhang Rui wrote:
> 
> > 
> > _BQC returns 3 when the actual backlight level is 0x50.
> > We only know that the value returned by _BQC is invalid but we never
> > know the value is an index until we see the _AML code.
> 
> If the value returned is less than the smallest value in the _BLC block, 
> then it's an index. This heuristic will never break a standards 
> compliant system, but (to the best of my knowledge) will work with any 
> of these systems that we're currently aware of.
> 
well, this does work, but a little bit ugly...
The ACPI backlight control on boxes with such _BCL/_BCM/_BQC methods
implemented surely doesn't work before.
so IMO disabling it again is not that bad. :)

thanks,
rui

Re: [PATCH] ACPI: disable the ACPI backlight control on laptops with buggy _BCL methods

[Matthew Garrett]

On Fri, Jan 09, 2009 at 09:53:45AM +0800, Zhang Rui wrote:

> > If the value returned is less than the smallest value in the _BLC block, 
> > then it's an index. This heuristic will never break a standards 
> > compliant system, but (to the best of my knowledge) will work with any 
> > of these systems that we're currently aware of.
> > 
> well, this does work, but a little bit ugly...
> The ACPI backlight control on boxes with such _BCL/_BCM/_BQC methods
> implemented surely doesn't work before.
> so IMO disabling it again is not that bad. :)

We don't know how widespread this behaviour is. If Vista can cope with 
it then we're likely to see machines that depend on it, so just hoping 
that we can always punt it to a platform driver doesn't sound like the 
best plan. If stock Vista is unable to deal with it then that's a 
definite argument in favour of leaving it up to platform drivers.

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