SRU update for acpi backlight problems
Andy Whitcroft
apw at canonical.com
Mon Jan 12 09:57:49 UTC 2009
On Sun, Jan 11, 2009 at 02:13:41PM +0100, Stefan Bader wrote:
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/311716
>
> SRU justification:
>
> Impact: Some laptops had issues with the current backlight control. This
> is because up to then acpi and vendor specific control methods were
> able/allowed to access the video hardware
> (https://bugs.launchpad.net/bugs/257827). The fix for this (taken from
> upstream) fixes this but causes regressions for others. So for stable it
> should probably be taken back but this would cause an ABI bump.
I would say it causes regressions for fewer people than that original
code. There was also a third set of apparent regressions but those were
regressions against the fix originally applied. By which I mean the
apparent regessions are not as bad as perhaps they seems.
> Fix: I changed the detection code in a (slightly ugly to keep diff
> minimal) way to make the old behaviour the default (generic _and_ vendor
> control at the same time) but leave the infrastructure (no ABI bump)
> which gives the additional ability for those that have problems with that
> to force either generic (video) or vendor specific (vendor) by using the
> module option acpi_backlight.
>
> Testcase: Boot the kernel and try to adjust backlight levels with FN-keys
> and/or the gnome backlight applet.
>
> Note: going over the original acpi patches, it looks like some vendor
> drivers might have been fixed wrong (eg. sony-laptop.c which bails out on
> !acpi_backlight_suppoert(). But that is the case when acpi is _not_
> active).
Looks like you fixed both acer-wmi and sony-laptop. From the context in
the patch they both look correct as fixed.
> --
>
> When all other means of communication fail, try words!
>
> ) which resulted in regression for some others (initial report here and
> in the other
> From 23bed0d21be3beba873e93fdf69bbd97768e246c Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader at canonical.com>
> Date: Fri, 9 Jan 2009 22:00:42 +0100
> Subject: [PATCH] UBUNTU: SAUCE: acpi: Hack to enable video and vendor backlight implementations
>
> Bug: #311716
>
> Some laptops had issues with the current backlight control. This is becausei
> up to then acpi and vendor specific control methods were able/allowed to
> access the video hardware (https://bugs.launchpad.net/bugs/257827).
> The fix for this (taken from upstream) fixes this but causes regressions for
> others. So for stable it should probably be taken back but this would cause
> an ABI bump.
> This changes the code in a way that keeps the new infrastructure but makes
> generic acpi video _and_ vendor backlight support the default (like it was
> before).
>
> Signed-off-by: Stefan Bader <stefan.bader at canonical.com>
> ---
> drivers/acpi/video.c | 2 +-
> drivers/acpi/video_detect.c | 19 +++++++++++++------
> drivers/misc/acer-wmi.c | 10 +++++-----
> drivers/misc/asus-laptop.c | 2 +-
> drivers/misc/compal-laptop.c | 2 +-
> drivers/misc/eeepc-laptop.c | 2 +-
> drivers/misc/fujitsu-laptop.c | 2 +-
> drivers/misc/msi-laptop.c | 2 +-
> drivers/misc/sony-laptop.c | 2 +-
> drivers/misc/thinkpad_acpi.c | 29 ++++++++++-------------------
> include/linux/acpi.h | 5 ++++-
> 11 files changed, 39 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index f3a5d99..c8c222a 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -739,7 +739,7 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
> device->cap._DSS = 1;
> }
>
> - if (acpi_video_backlight_support())
> + if (acpi_video_backlight_support() & ACPI_VIDEO_BACKLIGHT)
> max_level = acpi_video_init_brightness(device);
Probabally for an upstream merge we would wan to have like an
acpi_video_backlight_capabilities() which returns this bitmask, with:
int acpi_video_backlight_support(void
{
return acpi_video_backlight_capabilities() & ACPI_VIDEO_BACKLIGHT;
}
We cannot do that here without bumping the ABI presumably.
> if (device->cap._BCL && device->cap._BCM && max_level > 0) {
> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> index 70b1e91..e2e7c47 100644
> --- a/drivers/acpi/video_detect.c
> +++ b/drivers/acpi/video_detect.c
> @@ -179,7 +179,13 @@ long acpi_video_get_capabilities(acpi_handle graphics_handle)
> }
> EXPORT_SYMBOL(acpi_video_get_capabilities);
>
> -/* Returns true if video.ko can do backlight switching */
> +/*
> + * Temporary hack to enable a default of "both" which seemed to be true
> + * before. (LP#311716)
> + * Returns the bits ACPI_VIDEO_BACKLIGHT and/or
> + * ACPI_VIDEO_BACKLIGHT_FORCE_VENDORset to indicate which support should
> + * get activated.
> + */
Nit: missing space in the comment here.
> int acpi_video_backlight_support(void)
> {
> /*
> @@ -191,18 +197,19 @@ int acpi_video_backlight_support(void)
>
> /* First check for boot param -> highest prio */
> if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR)
> - return 0;
> + return ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
> else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO)
> - return 1;
> + return ACPI_VIDEO_BACKLIGHT;
>
> /* Then check for DMI blacklist -> second highest prio */
> if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VENDOR)
> - return 0;
> + return ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
> else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VIDEO)
> - return 1;
> + return ACPI_VIDEO_BACKLIGHT;
>
> /* Then go the default way */
> - return acpi_video_support & ACPI_VIDEO_BACKLIGHT;
> + return (acpi_video_support & ACPI_VIDEO_BACKLIGHT) |
> + ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
> }
> EXPORT_SYMBOL(acpi_video_backlight_support);
>
> diff --git a/drivers/misc/acer-wmi.c b/drivers/misc/acer-wmi.c
> index a4bef92..e63c2fd 100644
> --- a/drivers/misc/acer-wmi.c
> +++ b/drivers/misc/acer-wmi.c
> @@ -1242,11 +1242,11 @@ static int __init acer_wmi_init(void)
>
> set_quirks();
>
> - 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 (!acpi_vendor_backlight_support() && has_cap(ACER_CAP_BRIGHTNESS)) {
> + interface->capability &= ~ACER_CAP_BRIGHTNESS;
> + printk(ACER_INFO "Brightness must be controlled by "
> + "generic video driver\n");
> + }
This is BUG fix, and looks correct.
> if (platform_driver_register(&acer_platform_driver)) {
> printk(ACER_ERR "Unable to register platform driver.\n");
> diff --git a/drivers/misc/asus-laptop.c b/drivers/misc/asus-laptop.c
> index a9a823b..69d007a 100644
> --- a/drivers/misc/asus-laptop.c
> +++ b/drivers/misc/asus-laptop.c
> @@ -1207,7 +1207,7 @@ static int __init asus_laptop_init(void)
>
> dev = acpi_get_physical_device(hotk->device->handle);
>
> - if (!acpi_video_backlight_support()) {
> + if (acpi_vendor_backlight_support()) {
> result = asus_backlight_init(dev);
> if (result)
> goto fail_backlight;
> diff --git a/drivers/misc/compal-laptop.c b/drivers/misc/compal-laptop.c
> index 11003bb..27bea26 100644
> --- a/drivers/misc/compal-laptop.c
> +++ b/drivers/misc/compal-laptop.c
> @@ -326,7 +326,7 @@ static int __init compal_init(void)
>
> /* Register backlight stuff */
>
> - if (!acpi_video_backlight_support()) {
> + if (acpi_vendor_backlight_support()) {
> compalbl_device = backlight_device_register("compal-laptop", NULL, NULL,
> &compalbl_ops);
> if (IS_ERR(compalbl_device))
> diff --git a/drivers/misc/eeepc-laptop.c b/drivers/misc/eeepc-laptop.c
> index 50d42f2..aba2830 100644
> --- a/drivers/misc/eeepc-laptop.c
> +++ b/drivers/misc/eeepc-laptop.c
> @@ -636,7 +636,7 @@ static int __init eeepc_laptop_init(void)
> }
> dev = acpi_get_physical_device(ehotk->device->handle);
>
> - if (!acpi_video_backlight_support()) {
> + if (acpi_vendor_backlight_support()) {
> result = eeepc_backlight_init(dev);
> if (result)
> goto fail_backlight;
> diff --git a/drivers/misc/fujitsu-laptop.c b/drivers/misc/fujitsu-laptop.c
> index 255d135..c34818c 100644
> --- a/drivers/misc/fujitsu-laptop.c
> +++ b/drivers/misc/fujitsu-laptop.c
> @@ -970,7 +970,7 @@ static int __init fujitsu_init(void)
>
> /* Register backlight stuff */
>
> - if (!acpi_video_backlight_support()) {
> + if (acpi_vendor_backlight_support()) {
> fujitsu->bl_device =
> backlight_device_register("fujitsu-laptop", NULL, NULL,
> &fujitsubl_ops);
> diff --git a/drivers/misc/msi-laptop.c b/drivers/misc/msi-laptop.c
> index 759763d..9a30ee4 100644
> --- a/drivers/misc/msi-laptop.c
> +++ b/drivers/misc/msi-laptop.c
> @@ -347,7 +347,7 @@ static int __init msi_init(void)
>
> /* Register backlight stuff */
>
> - if (acpi_video_backlight_support()) {
> + if (!acpi_vendor_backlight_support()) {
> printk(KERN_INFO "MSI: Brightness ignored, must be controlled "
> "by ACPI video driver\n");
> } else {
> diff --git a/drivers/misc/sony-laptop.c b/drivers/misc/sony-laptop.c
> index 167f146..6dc77d8 100644
> --- a/drivers/misc/sony-laptop.c
> +++ b/drivers/misc/sony-laptop.c
> @@ -1038,7 +1038,7 @@ static int sony_nc_add(struct acpi_device *device)
> goto outinput;
> }
>
> - if (!acpi_video_backlight_support()) {
> + if (!acpi_vendor_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",
> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> index e5a020f..6b93007 100644
> --- a/drivers/misc/thinkpad_acpi.c
> +++ b/drivers/misc/thinkpad_acpi.c
> @@ -4921,25 +4921,16 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
> */
> b = tpacpi_check_std_acpi_brightness_support();
> if (b > 0) {
> -
> - 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 enabled, even if standard "
> - "ACPI backlight interface is available\n");
> - }
> - } else {
> - if (brightness_enable > 1) {
> - printk(TPACPI_NOTICE
> - "Standard ACPI backlight interface not "
> - "available, thinkpad_acpi native "
> - "brightness control enabled\n");
> - }
> + 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;
> }
> }
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 5e7e809..b10c11f 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -236,7 +236,7 @@ static inline long acpi_is_video_device(struct acpi_device *device)
>
> static inline int acpi_video_backlight_support(void)
> {
> - return 0;
> + return ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR;
> }
>
> static inline int acpi_video_display_switch_support(void)
> @@ -246,6 +246,9 @@ static inline int acpi_video_display_switch_support(void)
>
> #endif /* defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) */
>
> +#define acpi_vendor_backlight_support() \
> + (acpi_video_backlight_support() & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR)
> +
> 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.3
>
That fixes two clear bugs and looks like a sane approach giving us all
options. It might be worth mentioning how one selects in the leader of
the patch, with this option iiuc:
options video acpi_backlight=vendor
Assuming the testing shows the regressed people fixed this looks like a
good option over reverting everything.
ACK from me.
-apw
More information about the kernel-team
mailing list