| Message ID | 1231383496.20746.44.camel@rzhang-dt (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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; > > } -- 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
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.
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
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.
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
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.
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 -- 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
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.
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; }