Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   Ubuntu Kernel Team (http://www.linux-archive.org/ubuntu-kernel-team/)
-   -   SRU update for acpi backlight problems (http://www.linux-archive.org/ubuntu-kernel-team/225999-sru-update-acpi-backlight-problems.html)

Stefan Bader 01-11-2009 12:13 PM

SRU update for acpi backlight problems
 
This is a multi-part message in MIME format.
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.


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).


--

When all other means of communication fail, try words!

) which resulted in regression for some others (initial report here and in the
other

Stefan Bader 01-11-2009 12:35 PM

SRU update for acpi backlight problems
 
Stefan Bader wrote:
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/311716
>
> SRU justification:
>

I forgot, this is for the Intrepid update, but Jaunty code is in that state and
we would have to revert the latest change on both which tries to verify some
ACPI elements to decide whether to activate acpi generic support or not.

> 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.
>
> 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).
>


--

When all other means of communication fail, try words!



--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

Andy Whitcroft 01-12-2009 08:57 AM

SRU update for acpi backlight problems
 
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@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@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
");
> - }
> + 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
");
> + }

This is BUG fix, and looks correct.

> if (platform_driver_register(&acer_platform_driver)) {
> printk(ACER_ERR "Unable to register platform driver.
");
> 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
");
> } 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
");
> } 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.
");
> - return 1;
> - } else if (brightness_enable == 1) {
> - printk(TPACPI_NOTICE
> - "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 native "
> - "brightness control enabled
");
> - }
> + if (thinkpad_id.vendor == PCI_VENDOR_ID_LENOVO) {
> + printk(TPACPI_NOTICE
> + "Lenovo BIOS switched to ACPI backlight "
> + "control mode
");
> + }
> + if (brightness_enable > 1) {
> + printk(TPACPI_NOTICE
> + "standard ACPI backlight interface "
> + "available, not loading native one...
");
> + 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

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

Stefan Bader 01-12-2009 03:05 PM

SRU update for acpi backlight problems
 
This is a multi-part message in MIME format.
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.


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).




One addition to that: the T61 seems to be odd off. There is a change to fix
acpi from poking on the wrong video device. But then thinkpad_acpi detects this
is thee and backs of. Unfortunately generic support does not work as expected.
Probably because T61 in one of the rare machines that have _OSI=Linux enabled,
so the ACPI bios can act differently there...



--

When all other means of communication fail, try words!


All times are GMT. The time now is 08:19 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.