[PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

The AML implementation for brightness control on several ThinkPads
contains a workaround to meet a Windows 8 requirement of 101 brightness
levels [1]. The implementation is flawed, as only 16 of the brighness
values reported by _BCL affect a change in brightness. _BCM silently
discards the rest of the values. Disabling Windows 8 compatibility on
these machines reverts them to the old behavior, making _BCL only report
the 16 brightness levels which actually work. Add a quirk to do this
along with a dmi callback to disable Win8 compatibility.

[1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=51231
BugLink: http://bugs.launchpad.net/bugs/1098216
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---

This has been discussed previously at [2], however the description of
the problem there is incorrect. _BCM simply discards any value written
that isn't one of the brightness levels returned by _BCL for non-Win8
systems; it does not do any rounding.

This patch quirks six machines that have been confirmed to have this
problem, but there may be others. I looked for some way to do this other
than quirking for individual models, but unfortunately I couldn't find
any other criteria which worked.

Thanks,
Seth

[2] http://comments.gmane.org/gmane.linux.acpi.devel/58398


 drivers/acpi/blacklist.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c
index cb96296..6c242ed 100644
--- a/drivers/acpi/blacklist.c
+++ b/drivers/acpi/blacklist.c
@@ -193,6 +193,13 @@ static int __init dmi_disable_osi_win7(const struct dmi_system_id *d)
 	return 0;
 }
 
+static int __init dmi_disable_osi_win8(const struct dmi_system_id *d)
+{
+	printk(KERN_NOTICE PREFIX "DMI detected: %s\n", d->ident);
+	acpi_osi_setup("!Windows 2012");
+	return 0;
+}
+
 static struct dmi_system_id acpi_osi_dmi_table[] __initdata = {
 	{
 	.callback = dmi_disable_osi_vista,
@@ -269,6 +276,61 @@ static struct dmi_system_id acpi_osi_dmi_table[] __initdata = {
 	},
 
 	/*
+	 * The following Lenovo models have a broken workaround in the
+	 * acpi_video backlight implementation to meet the Windows 8
+	 * requirement of 101 backlight levels. Reverting to pre-Win8
+	 * behavior fixes the problem.
+	 */
+	{
+	.callback = dmi_disable_osi_win8,
+	.ident = "Lenovo ThinkPad L430",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad L430"),
+		},
+	},
+	{
+	.callback = dmi_disable_osi_win8,
+	.ident = "Lenovo ThinkPad T430s",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T430s"),
+		},
+	},
+	{
+	.callback = dmi_disable_osi_win8,
+	.ident = "Lenovo ThinkPad T530",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T530"),
+		},
+	},
+	{
+	.callback = dmi_disable_osi_win8,
+	.ident = "Lenovo ThinkPad W530",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad W530"),
+		},
+	},
+	{
+	.callback = dmi_disable_osi_win8,
+	.ident = "Lenovo ThinkPad X1 Carbon",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X1 Carbon"),
+		},
+	},
+	{
+	.callback = dmi_disable_osi_win8,
+	.ident = "Lenovo ThinkPad X230",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X230"),
+		},
+	},
+
+	/*
 	 * BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
 	 * Linux ignores it, except for the machines enumerated below.
 	 */
-- 
1.7.9.5

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Matthew Garrett]

On Mon, Feb 11, 2013 at 10:21:21AM -0600, Seth Forshee wrote:
> The AML implementation for brightness control on several ThinkPads
> contains a workaround to meet a Windows 8 requirement of 101 brightness
> levels [1]. The implementation is flawed, as only 16 of the brighness
> values reported by _BCL affect a change in brightness. _BCM silently
> discards the rest of the values. Disabling Windows 8 compatibility on
> these machines reverts them to the old behavior, making _BCL only report
> the 16 brightness levels which actually work. Add a quirk to do this
> along with a dmi callback to disable Win8 compatibility.

So the problem is that userspace is writing values that don't happen to 
be aligned with the values the hardware reacts to, and so nothing gets 
changed?

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

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Mon, Feb 11, 2013 at 05:52:13PM +0000, Matthew Garrett wrote:
> On Mon, Feb 11, 2013 at 10:21:21AM -0600, Seth Forshee wrote:
> > The AML implementation for brightness control on several ThinkPads
> > contains a workaround to meet a Windows 8 requirement of 101 brightness
> > levels [1]. The implementation is flawed, as only 16 of the brighness
> > values reported by _BCL affect a change in brightness. _BCM silently
> > discards the rest of the values. Disabling Windows 8 compatibility on
> > these machines reverts them to the old behavior, making _BCL only report
> > the 16 brightness levels which actually work. Add a quirk to do this
> > along with a dmi callback to disable Win8 compatibility.
> 
> So the problem is that userspace is writing values that don't happen to 
> be aligned with the values the hardware reacts to, and so nothing gets 
> changed?

Yes. The values are valid according to to _BCL, but _BCM is discarding
any values that aren't contained in an array named BRTW. BRTW is
literally the object returned by _BCL returns for !Windows 2012. Here's
a link to the AML if you'd like to take a look.

http://people.canonical.com/~sforshee/x230-acpi-tables/SSDT1.dsl

Seth

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Matthew Garrett]

On Mon, Feb 11, 2013 at 01:06:17PM -0600, Seth Forshee wrote:
> On Mon, Feb 11, 2013 at 05:52:13PM +0000, Matthew Garrett wrote:
> > So the problem is that userspace is writing values that don't happen to 
> > be aligned with the values the hardware reacts to, and so nothing gets 
> > changed?
> 
> Yes. The values are valid according to to _BCL, but _BCM is discarding
> any values that aren't contained in an array named BRTW. BRTW is
> literally the object returned by _BCL returns for !Windows 2012. Here's
> a link to the AML if you'd like to take a look.

Right. My concern here is that Windows clearly doesn't trigger the 
issue, and so there's some chance that we'll see similar issues on other 
machines. Disabling Windows 8 compatibility isn't really an option. One 
choice might be to have the ACPI video driver set all intermediate 
values if the system makes the Windows 8 OSI call?

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

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Rafael J. Wysocki]

On Monday, February 11, 2013 07:09:14 PM Matthew Garrett wrote:
> On Mon, Feb 11, 2013 at 01:06:17PM -0600, Seth Forshee wrote:
> > On Mon, Feb 11, 2013 at 05:52:13PM +0000, Matthew Garrett wrote:
> > > So the problem is that userspace is writing values that don't happen to 
> > > be aligned with the values the hardware reacts to, and so nothing gets 
> > > changed?
> > 
> > Yes. The values are valid according to to _BCL, but _BCM is discarding
> > any values that aren't contained in an array named BRTW. BRTW is
> > literally the object returned by _BCL returns for !Windows 2012. Here's
> > a link to the AML if you'd like to take a look.
> 
> Right. My concern here is that Windows clearly doesn't trigger the 
> issue, and so there's some chance that we'll see similar issues on other 
> machines. Disabling Windows 8 compatibility isn't really an option. One 
> choice might be to have the ACPI video driver set all intermediate 
> values if the system makes the Windows 8 OSI call?

At least I'd prefer that, so it would be great to verify if it works.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Mon, Feb 11, 2013 at 08:31:08PM +0100, Rafael J. Wysocki wrote:
> On Monday, February 11, 2013 07:09:14 PM Matthew Garrett wrote:
> > On Mon, Feb 11, 2013 at 01:06:17PM -0600, Seth Forshee wrote:
> > > On Mon, Feb 11, 2013 at 05:52:13PM +0000, Matthew Garrett wrote:
> > > > So the problem is that userspace is writing values that don't happen to 
> > > > be aligned with the values the hardware reacts to, and so nothing gets 
> > > > changed?
> > > 
> > > Yes. The values are valid according to to _BCL, but _BCM is discarding
> > > any values that aren't contained in an array named BRTW. BRTW is
> > > literally the object returned by _BCL returns for !Windows 2012. Here's
> > > a link to the AML if you'd like to take a look.
> > 
> > Right. My concern here is that Windows clearly doesn't trigger the 
> > issue, and so there's some chance that we'll see similar issues on other 
> > machines. Disabling Windows 8 compatibility isn't really an option. One 

So my take on this is that it's a transition issue. In the case of the
x230 I know that this machine was sold with Windows 7 before Windows 8
came out and that older versions of the firmware don't have the Win8
workaround. I suspect the story is the same with the other models.

> > choice might be to have the ACPI video driver set all intermediate 
> > values if the system makes the Windows 8 OSI call?
> 
> At least I'd prefer that, so it would be great to verify if it works.

I expect it will work, depending on your definition of "works."

The problem with this suggestion is that there are still going to be
instances where a user's desktop says the brightness has changed without
it actually changing. For example, gnome-settings-daemon is going to
break this down into increments of 5, and even with hitting every point
in between some steps aren't going to change the brightness at all.

But if disabling Windows 8 compatibility is out then that may be the
best we can do without resorting to something really ugly.

Seth

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Mon, Feb 11, 2013 at 07:09:14PM +0000, Matthew Garrett wrote:
> On Mon, Feb 11, 2013 at 01:06:17PM -0600, Seth Forshee wrote:
> > On Mon, Feb 11, 2013 at 05:52:13PM +0000, Matthew Garrett wrote:
> > > So the problem is that userspace is writing values that don't happen to 
> > > be aligned with the values the hardware reacts to, and so nothing gets 
> > > changed?
> > 
> > Yes. The values are valid according to to _BCL, but _BCM is discarding
> > any values that aren't contained in an array named BRTW. BRTW is
> > literally the object returned by _BCL returns for !Windows 2012. Here's
> > a link to the AML if you'd like to take a look.
> 
> Right. My concern here is that Windows clearly doesn't trigger the 
> issue, and so there's some chance that we'll see similar issues on other 
> machines. Disabling Windows 8 compatibility isn't really an option. One 
> choice might be to have the ACPI video driver set all intermediate 
> values if the system makes the Windows 8 OSI call?

This turns out to have some problems. The hotkeys on the x230 at least
generate increase/decrease brightness notifications. In response
acpi_video reads the current brightness level via _BQC and decides what
the next value should be. A value adjacent to a working value is never
another working value on this machine, so _BCM does nothing. The next
time a notification comes _BQC returns the same value as it did the
previous time. Obviously this gets us nowhere.

The (untested) fix I've come up with is to use the cached value for the
current brightness rather than asking the firmware. I'm assuming though
that acpi_video would be using the cached values already if there wasn't
a chance that their changing without its knowledge?

The other issue with using the cached value is that the hotkeys on these
machines are still going to end up cycling through 101 brightness levels
with 85% of them leaving the brightness unchanged. When there's that
many levels though maybe it makes sense to jump more than one level at a
time.

Seth

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Matthew Garrett]

On Wed, Feb 13, 2013 at 02:32:28PM -0600, Seth Forshee wrote:
> On Mon, Feb 11, 2013 at 07:09:14PM +0000, Matthew Garrett wrote:
> > Right. My concern here is that Windows clearly doesn't trigger the 
> > issue, and so there's some chance that we'll see similar issues on other 
> > machines. Disabling Windows 8 compatibility isn't really an option. One 
> > choice might be to have the ACPI video driver set all intermediate 
> > values if the system makes the Windows 8 OSI call?
> 
> This turns out to have some problems. The hotkeys on the x230 at least
> generate increase/decrease brightness notifications. In response
> acpi_video reads the current brightness level via _BQC and decides what
> the next value should be. A value adjacent to a working value is never
> another working value on this machine, so _BCM does nothing. The next
> time a notification comes _BQC returns the same value as it did the
> previous time. Obviously this gets us nowhere.

Nrgh. Having this logic in the kernel has always been miserable. On the 
other hand, having _BQC return wrong values is arguably even worse.

> The (untested) fix I've come up with is to use the cached value for the
> current brightness rather than asking the firmware. I'm assuming though
> that acpi_video would be using the cached values already if there wasn't
> a chance that their changing without its knowledge?

Yeah. What I'd suggest here is calling _BQC after every change, and if 
it returns the old value throw a firmware bug message and fall back to 
using a cached one.

> The other issue with using the cached value is that the hotkeys on these
> machines are still going to end up cycling through 101 brightness levels
> with 85% of them leaving the brightness unchanged. When there's that
> many levels though maybe it makes sense to jump more than one level at a
> time.

Right. I'd recommend turning off that functionality and just leaving it 
up to userspace. We still seem to be carrying a patch to do that in 
Fedora. I thought I had a patch to make this a config option somewhere, 
but can't find any sign of it now...

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

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Ben Jencks]

On 02/13/2013 03:55 PM, Matthew Garrett wrote:
> On Wed, Feb 13, 2013 at 02:32:28PM -0600, Seth Forshee wrote:
>> On Mon, Feb 11, 2013 at 07:09:14PM +0000, Matthew Garrett wrote:
>>> Right. My concern here is that Windows clearly doesn't trigger the 
>>> issue, and so there's some chance that we'll see similar issues on other 
>>> machines. Disabling Windows 8 compatibility isn't really an option. One 
>>> choice might be to have the ACPI video driver set all intermediate 
>>> values if the system makes the Windows 8 OSI call?

I'm not too familiar with the per-machine tweaks code; would it be
possible to override the SSDT1 table if it's this particular bad one?

>> The (untested) fix I've come up with is to use the cached value for the
>> current brightness rather than asking the firmware. I'm assuming though
>> that acpi_video would be using the cached values already if there wasn't
>> a chance that their changing without its knowledge?
> 
> Yeah. What I'd suggest here is calling _BQC after every change, and if 
> it returns the old value throw a firmware bug message and fall back to 
> using a cached one.

This still has the problem that some changes will have no effect, which
isn't a great user experience.

>> The other issue with using the cached value is that the hotkeys on these
>> machines are still going to end up cycling through 101 brightness levels
>> with 85% of them leaving the brightness unchanged. When there's that
>> many levels though maybe it makes sense to jump more than one level at a
>> time.
> 
> Right. I'd recommend turning off that functionality and just leaving it 
> up to userspace. We still seem to be carrying a patch to do that in 
> Fedora. I thought I had a patch to make this a config option somewhere, 
> but can't find any sign of it now...

I think you're looking for video.brightness_switch_enabled=0, which I
have to use, otherwise a single keypress changes brightness by two levels.

-Ben

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Ben Jencks]

On 02/11/2013 02:09 PM, Matthew Garrett wrote:
> On Mon, Feb 11, 2013 at 01:06:17PM -0600, Seth Forshee wrote:
>> On Mon, Feb 11, 2013 at 05:52:13PM +0000, Matthew Garrett wrote:
>>> So the problem is that userspace is writing values that don't happen to 
>>> be aligned with the values the hardware reacts to, and so nothing gets 
>>> changed?
>>
>> Yes. The values are valid according to to _BCL, but _BCM is discarding
>> any values that aren't contained in an array named BRTW. BRTW is
>> literally the object returned by _BCL returns for !Windows 2012. Here's
>> a link to the AML if you'd like to take a look.
> 
> Right. My concern here is that Windows clearly doesn't trigger the 
> issue, and so there's some chance that we'll see similar issues on other 
> machines. Disabling Windows 8 compatibility isn't really an option. One 
> choice might be to have the ACPI video driver set all intermediate 
> values if the system makes the Windows 8 OSI call?

Stupid hack idea: what about trying all the levels _BCL reports and
seeing which ones change _BQC? Then filter out any that don't really
exist. It might make bootup look odd, seeing the backlight quickly scan
through all brightness levels, but it's only once per boot.

-Ben

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Wed, Feb 13, 2013 at 08:55:58PM +0000, Matthew Garrett wrote:
> On Wed, Feb 13, 2013 at 02:32:28PM -0600, Seth Forshee wrote:
> > On Mon, Feb 11, 2013 at 07:09:14PM +0000, Matthew Garrett wrote:
> > > Right. My concern here is that Windows clearly doesn't trigger the 
> > > issue, and so there's some chance that we'll see similar issues on other 
> > > machines. Disabling Windows 8 compatibility isn't really an option. One 
> > > choice might be to have the ACPI video driver set all intermediate 
> > > values if the system makes the Windows 8 OSI call?
> > 
> > This turns out to have some problems. The hotkeys on the x230 at least
> > generate increase/decrease brightness notifications. In response
> > acpi_video reads the current brightness level via _BQC and decides what
> > the next value should be. A value adjacent to a working value is never
> > another working value on this machine, so _BCM does nothing. The next
> > time a notification comes _BQC returns the same value as it did the
> > previous time. Obviously this gets us nowhere.
> 
> Nrgh. Having this logic in the kernel has always been miserable. On the 
> other hand, having _BQC return wrong values is arguably even worse.
> 
> > The (untested) fix I've come up with is to use the cached value for the
> > current brightness rather than asking the firmware. I'm assuming though
> > that acpi_video would be using the cached values already if there wasn't
> > a chance that their changing without its knowledge?
> 
> Yeah. What I'd suggest here is calling _BQC after every change, and if 
> it returns the old value throw a firmware bug message and fall back to 
> using a cached one.

After every brightness change? Maybe it would be better to just test for
this during initialization, then set device->cap._BQC to 0 so we won't
use it.

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Wed, Feb 13, 2013 at 04:04:13PM -0500, Ben Jencks wrote:
> >> The other issue with using the cached value is that the hotkeys on these
> >> machines are still going to end up cycling through 101 brightness levels
> >> with 85% of them leaving the brightness unchanged. When there's that
> >> many levels though maybe it makes sense to jump more than one level at a
> >> time.
> > 
> > Right. I'd recommend turning off that functionality and just leaving it 
> > up to userspace. We still seem to be carrying a patch to do that in 
> > Fedora. I thought I had a patch to make this a config option somewhere, 
> > but can't find any sign of it now...
> 
> I think you're looking for video.brightness_switch_enabled=0, which I
> have to use, otherwise a single keypress changes brightness by two levels.

I think that option does what Matthew's suggesting. It would be a simple
change to default it to 0.

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Matthew Garrett]

On Wed, Feb 13, 2013 at 03:46:54PM -0600, Seth Forshee wrote:

> After every brightness change? Maybe it would be better to just test for
> this during initialization, then set device->cap._BQC to 0 so we won't
> use it.

That would require us to change the backlight during boot, which we 
don't necessarily want to do. There's also the risk that if we only do 
the check once it'll correspond to a valid value and we'll get a false 
positive.

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

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Wed, Feb 13, 2013 at 09:54:35PM +0000, Matthew Garrett wrote:
> On Wed, Feb 13, 2013 at 03:46:54PM -0600, Seth Forshee wrote:
> 
> > After every brightness change? Maybe it would be better to just test for
> > this during initialization, then set device->cap._BQC to 0 so we won't
> > use it.
> 
> That would require us to change the backlight during boot, which we 
> don't necessarily want to do. There's also the risk that if we only do 
> the check once it'll correspond to a valid value and we'll get a false 
> positive.

We're already setting the level during boot to check whether or not _BQC
returns brightness values or indexes. We'd obviously have to try several
values to be fairly certain that this bug wasn't present.

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Wed, Feb 13, 2013 at 08:55:58PM +0000, Matthew Garrett wrote:
> On Wed, Feb 13, 2013 at 02:32:28PM -0600, Seth Forshee wrote:
> > On Mon, Feb 11, 2013 at 07:09:14PM +0000, Matthew Garrett wrote:
> > > Right. My concern here is that Windows clearly doesn't trigger the 
> > > issue, and so there's some chance that we'll see similar issues on other 
> > > machines. Disabling Windows 8 compatibility isn't really an option. One 
> > > choice might be to have the ACPI video driver set all intermediate 
> > > values if the system makes the Windows 8 OSI call?
> > 
> > This turns out to have some problems. The hotkeys on the x230 at least
> > generate increase/decrease brightness notifications. In response
> > acpi_video reads the current brightness level via _BQC and decides what
> > the next value should be. A value adjacent to a working value is never
> > another working value on this machine, so _BCM does nothing. The next
> > time a notification comes _BQC returns the same value as it did the
> > previous time. Obviously this gets us nowhere.
> 
> Nrgh. Having this logic in the kernel has always been miserable. On the 
> other hand, having _BQC return wrong values is arguably even worse.
> 
> > The (untested) fix I've come up with is to use the cached value for the
> > current brightness rather than asking the firmware. I'm assuming though
> > that acpi_video would be using the cached values already if there wasn't
> > a chance that their changing without its knowledge?
> 
> Yeah. What I'd suggest here is calling _BQC after every change, and if 
> it returns the old value throw a firmware bug message and fall back to 
> using a cached one.
> 
> > The other issue with using the cached value is that the hotkeys on these
> > machines are still going to end up cycling through 101 brightness levels
> > with 85% of them leaving the brightness unchanged. When there's that
> > many levels though maybe it makes sense to jump more than one level at a
> > time.
> 
> Right. I'd recommend turning off that functionality and just leaving it 
> up to userspace. We still seem to be carrying a patch to do that in 
> Fedora. I thought I had a patch to make this a config option somewhere, 
> but can't find any sign of it now...

I've had the patches implementing your suggestions written for a while
now but just got test feedback this week. So here they are.

Seth

[PATCH 1/5] ACPICA: Add interface for getting latest Windows version requested via _OSI

[Seth Forshee]

The acpi_video driver needs to know the latest Windows version requested
by the BIOS to work around a buggy backlight implementation which is
specific to Windows 8. Add a new interface named
acpi_osi_windows_version() for retrieving this information.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/acpi/acpica/aclocal.h |   13 -------------
 drivers/acpi/acpica/utxface.c |   19 +++++++++++++++++++
 include/acpi/acpixf.h         |   18 ++++++++++++++++++
 3 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index ff8bd00..932c3d8 100644
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -926,19 +926,6 @@ struct acpi_bit_register_info {
 
 /* Structs and definitions for _OSI support and I/O port validation */
 
-#define ACPI_OSI_WIN_2000               0x01
-#define ACPI_OSI_WIN_XP                 0x02
-#define ACPI_OSI_WIN_XP_SP1             0x03
-#define ACPI_OSI_WINSRV_2003            0x04
-#define ACPI_OSI_WIN_XP_SP2             0x05
-#define ACPI_OSI_WINSRV_2003_SP1        0x06
-#define ACPI_OSI_WIN_VISTA              0x07
-#define ACPI_OSI_WINSRV_2008            0x08
-#define ACPI_OSI_WIN_VISTA_SP1          0x09
-#define ACPI_OSI_WIN_VISTA_SP2          0x0A
-#define ACPI_OSI_WIN_7                  0x0B
-#define ACPI_OSI_WIN_8                  0x0C
-
 #define ACPI_ALWAYS_ILLEGAL             0x00
 
 struct acpi_interface_info {
diff --git a/drivers/acpi/acpica/utxface.c b/drivers/acpi/acpica/utxface.c
index 390db0c..1531984 100644
--- a/drivers/acpi/acpica/utxface.c
+++ b/drivers/acpi/acpica/utxface.c
@@ -384,6 +384,25 @@ ACPI_EXPORT_SYMBOL(acpi_install_interface_handler)
 
 /*****************************************************************************
  *
+ * FUNCTION:    acpi_osi_windows_version
+ *
+ * PARAMETERS:  None
+ *
+ * RETURN:      Latest Windows version Windows requested via _OSI
+ *
+ * DESCRIPTION: Returns the latest version of Windows that has been requested
+ *              by the BIOS via _OSI.
+ *
+ ****************************************************************************/
+u8 acpi_osi_windows_version(void)
+{
+	return acpi_gbl_osi_data;
+}
+
+ACPI_EXPORT_SYMBOL(acpi_osi_windows_version)
+
+/*****************************************************************************
+ *
  * FUNCTION:    acpi_check_address_range
  *
  * PARAMETERS:  space_id            - Address space ID
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 3d88395..29a9429 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -73,6 +73,22 @@ extern u8 acpi_gbl_truncate_io_addresses;
 extern u8 acpi_gbl_disable_auto_repair;
 
 /*
+ * Values returned by acpi_osi_windows_version()
+ */
+#define ACPI_OSI_WIN_2000               0x01
+#define ACPI_OSI_WIN_XP                 0x02
+#define ACPI_OSI_WIN_XP_SP1             0x03
+#define ACPI_OSI_WINSRV_2003            0x04
+#define ACPI_OSI_WIN_XP_SP2             0x05
+#define ACPI_OSI_WINSRV_2003_SP1        0x06
+#define ACPI_OSI_WIN_VISTA              0x07
+#define ACPI_OSI_WINSRV_2008            0x08
+#define ACPI_OSI_WIN_VISTA_SP1          0x09
+#define ACPI_OSI_WIN_VISTA_SP2          0x0A
+#define ACPI_OSI_WIN_7                  0x0B
+#define ACPI_OSI_WIN_8                  0x0C
+
+/*
  * Hardware-reduced prototypes. All interfaces that use these macros will
  * be configured out of the ACPICA build if the ACPI_REDUCED_HARDWARE flag
  * is set to TRUE.
@@ -301,6 +317,8 @@ acpi_status acpi_install_notify_handler(acpi_handle device, u32 handler_type,
 					acpi_notify_handler handler,
 					void *context);
 
+u8 acpi_osi_windows_version(void);
+
 acpi_status
 acpi_remove_notify_handler(acpi_handle device,
 			   u32 handler_type, acpi_notify_handler handler);
-- 
1.7.9.5

[PATCH 2/5] acpi_video: Avoid unnecessary conversions between backlight levels and indexes

[Seth Forshee]

The acpi_video code deals mostly in backlight levels, but often these
levels are immediately converted to an index into the array of backlight
levels. This involves iterating over the array, whereas converting to
the index to a level is a simple index into the array.

Since the index to brightness conversion is a less expensive operation,
change the cached current brightness value to store the index rather
than the value. Also add versions of common brightness functions which
accept or return the index to avoid unnecessary conversions.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/acpi/video.c |  110 +++++++++++++++++++++++++++++---------------------
 1 file changed, 65 insertions(+), 45 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index f0e7e60..edfcd74 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -196,7 +196,7 @@ struct acpi_video_brightness_flags {
 };
 
 struct acpi_video_device_brightness {
-	int curr;
+	int curr_state;
 	int count;
 	int *levels;
 	struct acpi_video_brightness_flags flags;
@@ -226,11 +226,15 @@ static void acpi_video_device_rebind(struct acpi_video_bus *video);
 static void acpi_video_device_bind(struct acpi_video_bus *video,
 				   struct acpi_video_device *device);
 static int acpi_video_device_enumerate(struct acpi_video_bus *video);
+static int acpi_video_device_lcd_set_state(struct acpi_video_device *device,
+					   int state);
 static int acpi_video_device_lcd_set_level(struct acpi_video_device *device,
 			int level);
 static int acpi_video_device_lcd_get_level_current(
 			struct acpi_video_device *device,
 			unsigned long long *level, int init);
+static int acpi_video_device_lcd_get_state_current(
+			struct acpi_video_device *device, int *state);
 static int acpi_video_get_next_level(struct acpi_video_device *device,
 				     u32 level_current, u32 event);
 static int acpi_video_switch_brightness(struct acpi_video_device *device,
@@ -239,20 +243,15 @@ static int acpi_video_switch_brightness(struct acpi_video_device *device,
 /*backlight device sysfs support*/
 static int acpi_video_get_brightness(struct backlight_device *bd)
 {
-	unsigned long long cur_level;
-	int i;
+	int curr_state;
+	int result;
 	struct acpi_video_device *vd =
 		(struct acpi_video_device *)bl_get_data(bd);
 
-	if (acpi_video_device_lcd_get_level_current(vd, &cur_level, 0))
-		return -EINVAL;
-	for (i = 2; i < vd->brightness->count; i++) {
-		if (vd->brightness->levels[i] == cur_level)
-			/* The first two entries are special - see page 575
-			   of the ACPI spec 3.0 */
-			return i-2;
-	}
-	return 0;
+	result = acpi_video_device_lcd_get_state_current(vd, &curr_state);
+	if (result)
+		return result;
+	return curr_state - 2;
 }
 
 static int acpi_video_set_brightness(struct backlight_device *bd)
@@ -261,8 +260,7 @@ static int acpi_video_set_brightness(struct backlight_device *bd)
 	struct acpi_video_device *vd =
 		(struct acpi_video_device *)bl_get_data(bd);
 
-	return acpi_video_device_lcd_set_level(vd,
-				vd->brightness->levels[request_level]);
+	return acpi_video_device_lcd_set_state(vd, request_level);
 }
 
 static const struct backlight_ops acpi_backlight_ops = {
@@ -286,18 +284,15 @@ static int video_get_cur_state(struct thermal_cooling_device *cooling_dev, unsig
 {
 	struct acpi_device *device = cooling_dev->devdata;
 	struct acpi_video_device *video = acpi_driver_data(device);
-	unsigned long long level;
-	int offset;
+	int curr_state;
+	int result;
 
-	if (acpi_video_device_lcd_get_level_current(video, &level, 0))
-		return -EINVAL;
-	for (offset = 2; offset < video->brightness->count; offset++)
-		if (level == video->brightness->levels[offset]) {
-			*state = video->brightness->count - offset - 1;
-			return 0;
-		}
+	result = acpi_video_device_lcd_get_state_current(video, &curr_state);
+	if (result)
+		return result;
 
-	return -EINVAL;
+	*state = video->brightness->count - curr_state - 1;
+	return 0;
 }
 
 static int
@@ -305,14 +300,12 @@ video_set_cur_state(struct thermal_cooling_device *cooling_dev, unsigned long st
 {
 	struct acpi_device *device = cooling_dev->devdata;
 	struct acpi_video_device *video = acpi_driver_data(device);
-	int level;
 
 	if ( state >= video->brightness->count - 2)
 		return -EINVAL;
 
-	state = video->brightness->count - state;
-	level = video->brightness->levels[state -1];
-	return acpi_video_device_lcd_set_level(video, level);
+	state = video->brightness->count - state - 1;
+	return acpi_video_device_lcd_set_state(video, state);
 }
 
 static const struct thermal_cooling_device_ops video_cooling_ops = {
@@ -357,12 +350,12 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
 }
 
 static int
-acpi_video_device_lcd_set_level(struct acpi_video_device *device, int level)
+acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state)
 {
-	int status;
+	int level = device->brightness->levels[state];
 	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
 	struct acpi_object_list args = { 1, &arg0 };
-	int state;
+	acpi_status status;
 
 	arg0.integer.value = level;
 
@@ -373,16 +366,28 @@ acpi_video_device_lcd_set_level(struct acpi_video_device *device, int level)
 		return -EIO;
 	}
 
-	device->brightness->curr = level;
+	device->brightness->curr_state = state;
+	if (device->backlight)
+		device->backlight->props.brightness = state - 2;
+
+	return 0;
+}
+
+static int
+acpi_video_device_lcd_set_level(struct acpi_video_device *device, int level)
+{
+	int state;
+
 	for (state = 2; state < device->brightness->count; state++)
-		if (level == device->brightness->levels[state]) {
-			if (device->backlight)
-				device->backlight->props.brightness = state - 2;
-			return 0;
-		}
+		if (level == device->brightness->levels[state])
+			break;
 
-	ACPI_ERROR((AE_INFO, "Current brightness invalid"));
-	return -EINVAL;
+	if (state == device->brightness->count) {
+		ACPI_ERROR((AE_INFO, "Invalid brightness value"));
+		return -EINVAL;
+	}
+
+	return acpi_video_device_lcd_set_state(device, state);
 }
 
 /*
@@ -481,7 +486,7 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 			*level += bqc_offset_aml_bug_workaround;
 			for (i = 2; i < device->brightness->count; i++)
 				if (device->brightness->levels[i] == *level) {
-					device->brightness->curr = *level;
+					device->brightness->curr_state = i;
 					return 0;
 			}
 			if (!init) {
@@ -497,9 +502,10 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 		} else {
 			/* Fixme:
 			 * should we return an error or ignore this failure?
-			 * dev->brightness->curr is a cached value which stores
-			 * the correct current backlight level in most cases.
-			 * ACPI video backlight still works w/ buggy _BQC.
+			 * dev->brightness->curr_state is a cached value which
+			 * stores the index of correct current backlight level
+			 * in most cases. ACPI video backlight still works w/
+			 * buggy _BQC.
 			 * http://bugzilla.kernel.org/show_bug.cgi?id=12233
 			 */
 			ACPI_WARNING((AE_INFO, "Evaluating %s failed", buf));
@@ -507,11 +513,24 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 		}
 	}
 
-	*level = device->brightness->curr;
+	*level = device->brightness->levels[device->brightness->curr_state];
 	return 0;
 }
 
 static int
+acpi_video_device_lcd_get_state_current(struct acpi_video_device *device,
+					int *state)
+{
+	int result;
+	unsigned long long level;
+
+	result = acpi_video_device_lcd_get_level_current(device, &level, 0);
+
+	*state = device->brightness->curr_state;
+	return result;
+}
+
+static int
 acpi_video_device_EDID(struct acpi_video_device *device,
 		       union acpi_object **edid, ssize_t length)
 {
@@ -706,7 +725,8 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 	br->flags._BCM_use_index = br->flags._BCL_use_index;
 
 	/* _BQC uses INDEX while _BCL uses VALUE in some laptops */
-	br->curr = level = max_level;
+	level = max_level;
+	br->curr_state = count - 1;
 
 	if (!device->cap._BQC)
 		goto set_level;
-- 
1.7.9.5

[PATCH 3/5] acpi_video: Add workaround for broken Windows 8 backlight implementations

[Seth Forshee]

Windows 8 requires that all backlights report 101 brightness levels.
When Lenovo updated the firmware for some machines for Windows 8 they
met this requirement my making _BCL return a larger set of values for
Windows 8 than for other OSes. However, only the values in the smaller
set actually change the brightness at all. The rest of the values are
silently discarded.

As a workaround, change acpi_video to set all intermediate backlight
levels when setting the brightness. This isn't perfect, but it will mean
that most brightness changes done by common userspace utilities will hit
at least one valid brightness value.

[1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/acpi/video.c |   51 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index edfcd74..b83fbbd 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -352,25 +352,56 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
 static int
 acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state)
 {
-	int level = device->brightness->levels[state];
 	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
 	struct acpi_object_list args = { 1, &arg0 };
+	int curr_state, offset;
 	acpi_status status;
+	int result = 0;
 
-	arg0.integer.value = level;
+	curr_state = device->brightness->curr_state;
 
-	status = acpi_evaluate_object(device->dev->handle, "_BCM",
-				      &args, NULL);
-	if (ACPI_FAILURE(status)) {
-		ACPI_ERROR((AE_INFO, "Evaluating _BCM failed"));
-		return -EIO;
+	/*
+	 * Some Lenovo firmware has a broken backlight implementation
+	 * for Windows 8 where _BCL returns 101 backlight levels but
+	 * only 16 or so levels actually change the brightness at all.
+	 * As a workaround for these machines we set every intermediate
+	 * value between the old and new brightness levels whenever the
+	 * system has made the Windows 8 OSI call, hoping that at least
+	 * one of them will cause a change in brightness.
+	 */
+	if (acpi_osi_windows_version() == ACPI_OSI_WIN_8) {
+		if (state == curr_state)
+			offset = 0;
+		else
+			offset = state > curr_state ? 1 : -1;
+	} else {
+		offset = state - curr_state;
 	}
 
-	device->brightness->curr_state = state;
+	do {
+		curr_state += offset;
+		arg0.integer.value = device->brightness->levels[curr_state];
+
+		status = acpi_evaluate_object(device->dev->handle, "_BCM",
+					      &args, NULL);
+		if (ACPI_FAILURE(status)) {
+			ACPI_ERROR((AE_INFO, "Evaluating _BCM failed"));
+
+			/*
+			 * Change curr_state back to that of last
+			 * successful _BCM call
+			 */
+			curr_state -= offset;
+			result = -EIO;
+			break;
+		}
+	} while (curr_state != state);
+
+	device->brightness->curr_state = curr_state;
 	if (device->backlight)
-		device->backlight->props.brightness = state - 2;
+		device->backlight->props.brightness = curr_state - 2;
 
-	return 0;
+	return result;
 }
 
 static int
-- 
1.7.9.5

[PATCH 4/5] acpi_video: Disable use of _BQC when value doesn't match those set through _BCM

[Seth Forshee]

Check the value returned by _BQC after each brightness change, and if it
doesn't match the value written disable use of _BQC. This works around
problems on some Lenovos when valid brightness values are ignored by
_BCM.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/acpi/video.c |   59 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index b83fbbd..6a19bf7 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -230,6 +230,8 @@ static int acpi_video_device_lcd_set_state(struct acpi_video_device *device,
 					   int state);
 static int acpi_video_device_lcd_set_level(struct acpi_video_device *device,
 			int level);
+static int acpi_video_device_get_bqc_level(struct acpi_video_device *device,
+					   unsigned long long *level);
 static int acpi_video_device_lcd_get_level_current(
 			struct acpi_video_device *device,
 			unsigned long long *level, int init);
@@ -395,6 +397,20 @@ acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state)
 			result = -EIO;
 			break;
 		}
+
+		if (device->cap._BQC || device->cap._BCQ) {
+			int bqc_result;
+			unsigned long long level;
+			bqc_result = acpi_video_device_get_bqc_level(device,
+								     &level);
+			if (!bqc_result &&
+			    level != device->brightness->levels[curr_state]) {
+				printk(KERN_WARNING FW_BUG
+				       "%s level does not match level set; disabling\n",
+				       device->cap._BQC ? "_BQC" : "_BCQ");
+				device->cap._BQC = device->cap._BCQ = 0;
+			}
+		}
 	} while (curr_state != state);
 
 	device->brightness->curr_state = curr_state;
@@ -495,26 +511,36 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
 };
 
 static int
+acpi_video_device_get_bqc_level(struct acpi_video_device *device,
+				      unsigned long long *level)
+{
+	acpi_status status;
+	char *buf = device->cap._BQC ? "_BQC" : "_BCQ";
+
+	status = acpi_evaluate_integer(device->dev->handle, buf, NULL, level);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	if (device->brightness->flags._BQC_use_index) {
+		if (device->brightness->flags._BCL_reversed)
+			*level = device->brightness->count - 3 - *level;
+		*level = device->brightness->levels[*level + 2];
+	}
+
+	*level += bqc_offset_aml_bug_workaround;
+	return 0;
+}
+
+static int
 acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 					unsigned long long *level, int init)
 {
-	acpi_status status = AE_OK;
+	int result;
 	int i;
 
 	if (device->cap._BQC || device->cap._BCQ) {
-		char *buf = device->cap._BQC ? "_BQC" : "_BCQ";
-
-		status = acpi_evaluate_integer(device->dev->handle, buf,
-						NULL, level);
-		if (ACPI_SUCCESS(status)) {
-			if (device->brightness->flags._BQC_use_index) {
-				if (device->brightness->flags._BCL_reversed)
-					*level = device->brightness->count
-								 - 3 - (*level);
-				*level = device->brightness->levels[*level + 2];
-
-			}
-			*level += bqc_offset_aml_bug_workaround;
+		result = acpi_video_device_get_bqc_level(device, level);
+		if (!result) {
 			for (i = 2; i < device->brightness->count; i++)
 				if (device->brightness->levels[i] == *level) {
 					device->brightness->curr_state = i;
@@ -527,7 +553,7 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 				 */
 				ACPI_WARNING((AE_INFO,
 					      "%s returned an invalid level",
-					      buf));
+					      device->cap._BQC ? "_BQC" : "_BCQ"));
 				device->cap._BQC = device->cap._BCQ = 0;
 			}
 		} else {
@@ -539,7 +565,8 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 			 * buggy _BQC.
 			 * http://bugzilla.kernel.org/show_bug.cgi?id=12233
 			 */
-			ACPI_WARNING((AE_INFO, "Evaluating %s failed", buf));
+			ACPI_WARNING((AE_INFO, "Evaluating %s failed",
+				      device->cap._BQC ? "_BQC" : "_BCQ"));
 			device->cap._BQC = device->cap._BCQ = 0;
 		}
 	}
-- 
1.7.9.5

[PATCH 5/5] acpi_video: Don't handle ACPI brightness notifications by default

[Seth Forshee]

Windows 8 requires all backlight interfaces to report 101 brightness
values, and as a result we're starting to see machines with that many
brightness levels in _BCL. For machines which send these notifications
when the brightness up/down keys are pressed this means a lot of key
presses to get any kind of noticeable change in brightness.

For a while now we've had the ability to disable in-kernel handling of
notifications via the video.brightness_switch_enabled parameter. Change
this to default to off, and let userspace choose more reasonable
increments for changing the brightness.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/acpi/video.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 6a19bf7..431b22e 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -69,7 +69,7 @@ MODULE_AUTHOR("Bruno Ducrot");
 MODULE_DESCRIPTION("ACPI Video Driver");
 MODULE_LICENSE("GPL");
 
-static bool brightness_switch_enabled = 1;
+static bool brightness_switch_enabled = 0;
 module_param(brightness_switch_enabled, bool, 0644);
 
 /*
-- 
1.7.9.5

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Thu, Mar 07, 2013 at 01:38:12PM -0600, Seth Forshee wrote:
> On Wed, Feb 13, 2013 at 08:55:58PM +0000, Matthew Garrett wrote:
> > On Wed, Feb 13, 2013 at 02:32:28PM -0600, Seth Forshee wrote:
> > > On Mon, Feb 11, 2013 at 07:09:14PM +0000, Matthew Garrett wrote:
> > > > Right. My concern here is that Windows clearly doesn't trigger the 
> > > > issue, and so there's some chance that we'll see similar issues on other 
> > > > machines. Disabling Windows 8 compatibility isn't really an option. One 
> > > > choice might be to have the ACPI video driver set all intermediate 
> > > > values if the system makes the Windows 8 OSI call?
> > > 
> > > This turns out to have some problems. The hotkeys on the x230 at least
> > > generate increase/decrease brightness notifications. In response
> > > acpi_video reads the current brightness level via _BQC and decides what
> > > the next value should be. A value adjacent to a working value is never
> > > another working value on this machine, so _BCM does nothing. The next
> > > time a notification comes _BQC returns the same value as it did the
> > > previous time. Obviously this gets us nowhere.
> > 
> > Nrgh. Having this logic in the kernel has always been miserable. On the 
> > other hand, having _BQC return wrong values is arguably even worse.
> > 
> > > The (untested) fix I've come up with is to use the cached value for the
> > > current brightness rather than asking the firmware. I'm assuming though
> > > that acpi_video would be using the cached values already if there wasn't
> > > a chance that their changing without its knowledge?
> > 
> > Yeah. What I'd suggest here is calling _BQC after every change, and if 
> > it returns the old value throw a firmware bug message and fall back to 
> > using a cached one.
> > 
> > > The other issue with using the cached value is that the hotkeys on these
> > > machines are still going to end up cycling through 101 brightness levels
> > > with 85% of them leaving the brightness unchanged. When there's that
> > > many levels though maybe it makes sense to jump more than one level at a
> > > time.
> > 
> > Right. I'd recommend turning off that functionality and just leaving it 
> > up to userspace. We still seem to be carrying a patch to do that in 
> > Fedora. I thought I had a patch to make this a config option somewhere, 
> > but can't find any sign of it now...
> 
> I've had the patches implementing your suggestions written for a while
> now but just got test feedback this week. So here they are.

Ping.

Matthew, do these patches look like what you had in mind?

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 02/12/2013 12:21 AM, Seth Forshee wrote:
> The AML implementation for brightness control on several ThinkPads
> contains a workaround to meet a Windows 8 requirement of 101 brightness
> levels [1]. The implementation is flawed, as only 16 of the brighness
> values reported by _BCL affect a change in brightness. _BCM silently
> discards the rest of the values. Disabling Windows 8 compatibility on
> these machines reverts them to the old behavior, making _BCL only report
> the 16 brightness levels which actually work. Add a quirk to do this
> along with a dmi callback to disable Win8 compatibility.

If we disable the _BQC(i.e. set cap._BQC=0) for these systems, will the
problem go away? If so, I think perhaps we can put these systems into a
_BQC quirk table and set cap._BQC=0 for them.

Thanks,
Aaron

> 
> [1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=51231
> BugLink: http://bugs.launchpad.net/bugs/1098216
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
> 
> This has been discussed previously at [2], however the description of
> the problem there is incorrect. _BCM simply discards any value written
> that isn't one of the brightness levels returned by _BCL for non-Win8
> systems; it does not do any rounding.
> 
> This patch quirks six machines that have been confirmed to have this
> problem, but there may be others. I looked for some way to do this other
> than quirking for individual models, but unfortunately I couldn't find
> any other criteria which worked.
> 
> Thanks,
> Seth
> 
> [2] http://comments.gmane.org/gmane.linux.acpi.devel/58398
> 
> 
>  drivers/acpi/blacklist.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c
> index cb96296..6c242ed 100644
> --- a/drivers/acpi/blacklist.c
> +++ b/drivers/acpi/blacklist.c
> @@ -193,6 +193,13 @@ static int __init dmi_disable_osi_win7(const struct dmi_system_id *d)
>  	return 0;
>  }
>  
> +static int __init dmi_disable_osi_win8(const struct dmi_system_id *d)
> +{
> +	printk(KERN_NOTICE PREFIX "DMI detected: %s\n", d->ident);
> +	acpi_osi_setup("!Windows 2012");
> +	return 0;
> +}
> +
>  static struct dmi_system_id acpi_osi_dmi_table[] __initdata = {
>  	{
>  	.callback = dmi_disable_osi_vista,
> @@ -269,6 +276,61 @@ static struct dmi_system_id acpi_osi_dmi_table[] __initdata = {
>  	},
>  
>  	/*
> +	 * The following Lenovo models have a broken workaround in the
> +	 * acpi_video backlight implementation to meet the Windows 8
> +	 * requirement of 101 backlight levels. Reverting to pre-Win8
> +	 * behavior fixes the problem.
> +	 */
> +	{
> +	.callback = dmi_disable_osi_win8,
> +	.ident = "Lenovo ThinkPad L430",
> +	.matches = {
> +		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +		     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad L430"),
> +		},
> +	},
> +	{
> +	.callback = dmi_disable_osi_win8,
> +	.ident = "Lenovo ThinkPad T430s",
> +	.matches = {
> +		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +		     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T430s"),
> +		},
> +	},
> +	{
> +	.callback = dmi_disable_osi_win8,
> +	.ident = "Lenovo ThinkPad T530",
> +	.matches = {
> +		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +		     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T530"),
> +		},
> +	},
> +	{
> +	.callback = dmi_disable_osi_win8,
> +	.ident = "Lenovo ThinkPad W530",
> +	.matches = {
> +		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +		     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad W530"),
> +		},
> +	},
> +	{
> +	.callback = dmi_disable_osi_win8,
> +	.ident = "Lenovo ThinkPad X1 Carbon",
> +	.matches = {
> +		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +		     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X1 Carbon"),
> +		},
> +	},
> +	{
> +	.callback = dmi_disable_osi_win8,
> +	.ident = "Lenovo ThinkPad X230",
> +	.matches = {
> +		     DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +		     DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X230"),
> +		},
> +	},
> +
> +	/*
>  	 * BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
>  	 * Linux ignores it, except for the machines enumerated below.
>  	 */
> 

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Mon, Apr 01, 2013 at 09:53:36AM +0800, Aaron Lu wrote:
> On 02/12/2013 12:21 AM, Seth Forshee wrote:
> > The AML implementation for brightness control on several ThinkPads
> > contains a workaround to meet a Windows 8 requirement of 101 brightness
> > levels [1]. The implementation is flawed, as only 16 of the brighness
> > values reported by _BCL affect a change in brightness. _BCM silently
> > discards the rest of the values. Disabling Windows 8 compatibility on
> > these machines reverts them to the old behavior, making _BCL only report
> > the 16 brightness levels which actually work. Add a quirk to do this
> > along with a dmi callback to disable Win8 compatibility.
> 
> If we disable the _BQC(i.e. set cap._BQC=0) for these systems, will the
> problem go away? If so, I think perhaps we can put these systems into a
> _BQC quirk table and set cap._BQC=0 for them.

That helps a little, but we're still left with only 16 of the 101
brightness levels causing any change in brightness. The firmware isn't
rounding the "bad" values or anything like that; it just silently
ignores them.

I submitted a second set of patches [1] which writes all intermediate
values between the old and new brightness values and disables _BQC for
these machines (empirically rather than using a quirk table), though no
one seems to be interested in reviewing them.

Seth

[1] http://www.spinics.net/lists/linux-acpi/msg42525.html

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Ben Jencks]

On 03/18/2013 05:25 PM, Seth Forshee wrote:
> On Thu, Mar 07, 2013 at 01:38:12PM -0600, Seth Forshee wrote:
>> On Wed, Feb 13, 2013 at 08:55:58PM +0000, Matthew Garrett wrote:
>>> On Wed, Feb 13, 2013 at 02:32:28PM -0600, Seth Forshee wrote:
>>>> On Mon, Feb 11, 2013 at 07:09:14PM +0000, Matthew Garrett wrote:
>>>>> Right. My concern here is that Windows clearly doesn't trigger the 
>>>>> issue, and so there's some chance that we'll see similar issues on other 
>>>>> machines. Disabling Windows 8 compatibility isn't really an option. One 
>>>>> choice might be to have the ACPI video driver set all intermediate 
>>>>> values if the system makes the Windows 8 OSI call?
>>>>
>>>> This turns out to have some problems. The hotkeys on the x230 at least
>>>> generate increase/decrease brightness notifications. In response
>>>> acpi_video reads the current brightness level via _BQC and decides what
>>>> the next value should be. A value adjacent to a working value is never
>>>> another working value on this machine, so _BCM does nothing. The next
>>>> time a notification comes _BQC returns the same value as it did the
>>>> previous time. Obviously this gets us nowhere.
>>>
>>> Nrgh. Having this logic in the kernel has always been miserable. On the 
>>> other hand, having _BQC return wrong values is arguably even worse.
>>>
>>>> The (untested) fix I've come up with is to use the cached value for the
>>>> current brightness rather than asking the firmware. I'm assuming though
>>>> that acpi_video would be using the cached values already if there wasn't
>>>> a chance that their changing without its knowledge?
>>>
>>> Yeah. What I'd suggest here is calling _BQC after every change, and if 
>>> it returns the old value throw a firmware bug message and fall back to 
>>> using a cached one.
>>>
>>>> The other issue with using the cached value is that the hotkeys on these
>>>> machines are still going to end up cycling through 101 brightness levels
>>>> with 85% of them leaving the brightness unchanged. When there's that
>>>> many levels though maybe it makes sense to jump more than one level at a
>>>> time.
>>>
>>> Right. I'd recommend turning off that functionality and just leaving it 
>>> up to userspace. We still seem to be carrying a patch to do that in 
>>> Fedora. I thought I had a patch to make this a config option somewhere, 
>>> but can't find any sign of it now...
>>
>> I've had the patches implementing your suggestions written for a while
>> now but just got test feedback this week. So here they are.
> 
> Ping.
> 
> Matthew, do these patches look like what you had in mind?

Finally got a chance to try these patches out. It seems to perform as
expected, exposing levels 0-100 and actually allowing proper changes
between them. It's not optimal, though.

Using the up/down keys with gnome-settings-daemon, which uses 20 steps,
several steps don't actually change the brightness (including the very
first one, 100->95). Additionally, the changes are asymmetric: going
down, the brightness changes at 95->90, but going up 90->95 has no
change, and 95->100 changes it.

As a user, I'd definitely consider this a regression compared to the
"!Windows 2012" behavior. If you can't remove that OSI or override the
_BCL list as a machine-specific quirk, this is probably the best generic
behavior possible, though.

-Ben

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/01/2013 09:03 PM, Seth Forshee wrote:
> On Mon, Apr 01, 2013 at 09:53:36AM +0800, Aaron Lu wrote:
>> On 02/12/2013 12:21 AM, Seth Forshee wrote:
>>> The AML implementation for brightness control on several ThinkPads
>>> contains a workaround to meet a Windows 8 requirement of 101 brightness
>>> levels [1]. The implementation is flawed, as only 16 of the brighness
>>> values reported by _BCL affect a change in brightness. _BCM silently
>>> discards the rest of the values. Disabling Windows 8 compatibility on
>>> these machines reverts them to the old behavior, making _BCL only report
>>> the 16 brightness levels which actually work. Add a quirk to do this
>>> along with a dmi callback to disable Win8 compatibility.
>>
>> If we disable the _BQC(i.e. set cap._BQC=0) for these systems, will the
>> problem go away? If so, I think perhaps we can put these systems into a
>> _BQC quirk table and set cap._BQC=0 for them.
> 
> That helps a little, but we're still left with only 16 of the 101
> brightness levels causing any change in brightness. The firmware isn't
> rounding the "bad" values or anything like that; it just silently
> ignores them.

OK, and if GUI tool like g-s-d decides to go some more steps when
adjusting backlight level, it may always choose the non-functional
values. Hmm, doesn't seem to be an usable way then.

I really wondered, how Windows handled this, it should have the same
problem, unless they are not using the acpi video interface?

> 
> I submitted a second set of patches [1] which writes all intermediate
> values between the old and new brightness values and disables _BQC for
> these machines (empirically rather than using a quirk table), though no
> one seems to be interested in reviewing them.

Suppose we are at level 100, and the user sets the target level to be
20, then we will need to call _BCM 80 times?

Thanks,
Aaron

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/02/2013 01:18 PM, Ben Jencks wrote:
> On 03/18/2013 05:25 PM, Seth Forshee wrote:
>> On Thu, Mar 07, 2013 at 01:38:12PM -0600, Seth Forshee wrote:
>>> On Wed, Feb 13, 2013 at 08:55:58PM +0000, Matthew Garrett wrote:
>>>> On Wed, Feb 13, 2013 at 02:32:28PM -0600, Seth Forshee wrote:
>>>>> On Mon, Feb 11, 2013 at 07:09:14PM +0000, Matthew Garrett wrote:
>>>>>> Right. My concern here is that Windows clearly doesn't trigger the 
>>>>>> issue, and so there's some chance that we'll see similar issues on other 
>>>>>> machines. Disabling Windows 8 compatibility isn't really an option. One 
>>>>>> choice might be to have the ACPI video driver set all intermediate 
>>>>>> values if the system makes the Windows 8 OSI call?
>>>>>
>>>>> This turns out to have some problems. The hotkeys on the x230 at least
>>>>> generate increase/decrease brightness notifications. In response
>>>>> acpi_video reads the current brightness level via _BQC and decides what
>>>>> the next value should be. A value adjacent to a working value is never
>>>>> another working value on this machine, so _BCM does nothing. The next
>>>>> time a notification comes _BQC returns the same value as it did the
>>>>> previous time. Obviously this gets us nowhere.
>>>>
>>>> Nrgh. Having this logic in the kernel has always been miserable. On the 
>>>> other hand, having _BQC return wrong values is arguably even worse.
>>>>
>>>>> The (untested) fix I've come up with is to use the cached value for the
>>>>> current brightness rather than asking the firmware. I'm assuming though
>>>>> that acpi_video would be using the cached values already if there wasn't
>>>>> a chance that their changing without its knowledge?
>>>>
>>>> Yeah. What I'd suggest here is calling _BQC after every change, and if 
>>>> it returns the old value throw a firmware bug message and fall back to 
>>>> using a cached one.
>>>>
>>>>> The other issue with using the cached value is that the hotkeys on these
>>>>> machines are still going to end up cycling through 101 brightness levels
>>>>> with 85% of them leaving the brightness unchanged. When there's that
>>>>> many levels though maybe it makes sense to jump more than one level at a
>>>>> time.
>>>>
>>>> Right. I'd recommend turning off that functionality and just leaving it 
>>>> up to userspace. We still seem to be carrying a patch to do that in 
>>>> Fedora. I thought I had a patch to make this a config option somewhere, 
>>>> but can't find any sign of it now...
>>>
>>> I've had the patches implementing your suggestions written for a while
>>> now but just got test feedback this week. So here they are.
>>
>> Ping.
>>
>> Matthew, do these patches look like what you had in mind?
> 
> Finally got a chance to try these patches out. It seems to perform as
> expected, exposing levels 0-100 and actually allowing proper changes
> between them. It's not optimal, though.
> 
> Using the up/down keys with gnome-settings-daemon, which uses 20 steps,
> several steps don't actually change the brightness (including the very
> first one, 100->95). Additionally, the changes are asymmetric: going
> down, the brightness changes at 95->90, but going up 90->95 has no
> change, and 95->100 changes it.
> 
> As a user, I'd definitely consider this a regression compared to the
> "!Windows 2012" behavior. If you can't remove that OSI or override the
> _BCL list as a machine-specific quirk, this is probably the best generic
> behavior possible, though.

Perhaps we can introduce a _BCM quirk function, with a DMI table for
these systems. On boot/load, the callback of the dmi entry would to
evaluate for which values _BCM has effect by checking with _BQC, and
re-construct the _BCL table according to this.

This has the benefit of not affecting other systems, while also derive
the correct table for _BCL for these broken systems.

I saw you guys have talked about this idea in this thread, so I wonder
if this is a viable solution?

Thanks,
Aaron

> 
> -Ben
> --
> 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 Windows 8 compatibility for some Lenovo ThinkPads

[Matthew Garrett]

On Tue, Apr 02, 2013 at 05:15:03PM +0800, Aaron Lu wrote:

> Perhaps we can introduce a _BCM quirk function, with a DMI table for
> these systems.

No. Really, plesae no.

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

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
> On 04/01/2013 09:03 PM, Seth Forshee wrote:
> > On Mon, Apr 01, 2013 at 09:53:36AM +0800, Aaron Lu wrote:
> >> On 02/12/2013 12:21 AM, Seth Forshee wrote:
> >>> The AML implementation for brightness control on several ThinkPads
> >>> contains a workaround to meet a Windows 8 requirement of 101 brightness
> >>> levels [1]. The implementation is flawed, as only 16 of the brighness
> >>> values reported by _BCL affect a change in brightness. _BCM silently
> >>> discards the rest of the values. Disabling Windows 8 compatibility on
> >>> these machines reverts them to the old behavior, making _BCL only report
> >>> the 16 brightness levels which actually work. Add a quirk to do this
> >>> along with a dmi callback to disable Win8 compatibility.
> >>
> >> If we disable the _BQC(i.e. set cap._BQC=0) for these systems, will the
> >> problem go away? If so, I think perhaps we can put these systems into a
> >> _BQC quirk table and set cap._BQC=0 for them.
> > 
> > That helps a little, but we're still left with only 16 of the 101
> > brightness levels causing any change in brightness. The firmware isn't
> > rounding the "bad" values or anything like that; it just silently
> > ignores them.
> 
> OK, and if GUI tool like g-s-d decides to go some more steps when
> adjusting backlight level, it may always choose the non-functional
> values. Hmm, doesn't seem to be an usable way then.

Exactly.

> I really wondered, how Windows handled this, it should have the same
> problem, unless they are not using the acpi video interface?

I can only guess.

I think I remember reading that Windows 8 does smooth backlight
transitions, so it may well hit every intermediate brightness value.
Lenovo could also be supplying a driver which rounds values to the
nearest working value or uses some other interface or something else.

> > I submitted a second set of patches [1] which writes all intermediate
> > values between the old and new brightness values and disables _BQC for
> > these machines (empirically rather than using a quirk table), though no
> > one seems to be interested in reviewing them.
> 
> Suppose we are at level 100, and the user sets the target level to be
> 20, then we will need to call _BCM 80 times?

Yes. And on machines where _BQC actually returns legitimate values it
will get called 80 times as well. On these Lenovo's we'd quickly
determine that _BQC doesn't work and stop trying to use it however.

Seth

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/02/2013 09:00 PM, Seth Forshee wrote:
> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
>> On 04/01/2013 09:03 PM, Seth Forshee wrote:
>>> On Mon, Apr 01, 2013 at 09:53:36AM +0800, Aaron Lu wrote:
>>>> On 02/12/2013 12:21 AM, Seth Forshee wrote:
>>>>> The AML implementation for brightness control on several ThinkPads
>>>>> contains a workaround to meet a Windows 8 requirement of 101 brightness
>>>>> levels [1]. The implementation is flawed, as only 16 of the brighness
>>>>> values reported by _BCL affect a change in brightness. _BCM silently
>>>>> discards the rest of the values. Disabling Windows 8 compatibility on
>>>>> these machines reverts them to the old behavior, making _BCL only report
>>>>> the 16 brightness levels which actually work. Add a quirk to do this
>>>>> along with a dmi callback to disable Win8 compatibility.
>>>>
>>>> If we disable the _BQC(i.e. set cap._BQC=0) for these systems, will the
>>>> problem go away? If so, I think perhaps we can put these systems into a
>>>> _BQC quirk table and set cap._BQC=0 for them.
>>>
>>> That helps a little, but we're still left with only 16 of the 101
>>> brightness levels causing any change in brightness. The firmware isn't
>>> rounding the "bad" values or anything like that; it just silently
>>> ignores them.
>>
>> OK, and if GUI tool like g-s-d decides to go some more steps when
>> adjusting backlight level, it may always choose the non-functional
>> values. Hmm, doesn't seem to be an usable way then.
> 
> Exactly.
> 
>> I really wondered, how Windows handled this, it should have the same
>> problem, unless they are not using the acpi video interface?
> 
> I can only guess.
> 
> I think I remember reading that Windows 8 does smooth backlight
> transitions, so it may well hit every intermediate brightness value.
> Lenovo could also be supplying a driver which rounds values to the
> nearest working value or uses some other interface or something else.

Right, thanks for the hint.

> 
>>> I submitted a second set of patches [1] which writes all intermediate
>>> values between the old and new brightness values and disables _BQC for
>>> these machines (empirically rather than using a quirk table), though no
>>> one seems to be interested in reviewing them.
>>
>> Suppose we are at level 100, and the user sets the target level to be
>> 20, then we will need to call _BCM 80 times?
> 
> Yes. And on machines where _BQC actually returns legitimate values it
> will get called 80 times as well. On these Lenovo's we'd quickly
> determine that _BQC doesn't work and stop trying to use it however.

OK, but we still have to call it 80 times, right? From smooth point of
view, this might be a good thing, but I'm not sure if such change of
behaviour just for this problem is desired.

And I just took a look at patch 3, it seems to affect the normal code
path too, so we are changing the behaviour of a brightness level change
for some specific systems.

I really do not want these quirk code to pollute normal code path, it
made code difficult to understand and complicated. And all systems
suffer from this change, not just the affected ones.

I hope we can use quirk to differentiate them.

Thanks,
Aaron

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/02/2013 07:23 PM, Matthew Garrett wrote:
> On Tue, Apr 02, 2013 at 05:15:03PM +0800, Aaron Lu wrote:
> 
>> Perhaps we can introduce a _BCM quirk function, with a DMI table for
>> these systems.
> 
> No. Really, plesae no.
> 

I would like to know why, care to explain?

Thanks,
Aaron

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Matthew Garrett]

On Tue, Apr 02, 2013 at 09:44:07PM +0800, Aaron Lu wrote:
> On 04/02/2013 07:23 PM, Matthew Garrett wrote:
> > On Tue, Apr 02, 2013 at 05:15:03PM +0800, Aaron Lu wrote:
> > 
> >> Perhaps we can introduce a _BCM quirk function, with a DMI table for
> >> these systems.
> > 
> > No. Really, plesae no.
> > 
> 
> I would like to know why, care to explain?

Because Lenovo release another machine with the same behavour and it's 
broken until someone notices and files a bug and gets it added to a 
list. DMI blacklisting is almost always the wrong answer.

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

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Ben Jencks]

On 04/02/2013 09:00 AM, Seth Forshee wrote:
> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
>> On 04/01/2013 09:03 PM, Seth Forshee wrote:
>>> On Mon, Apr 01, 2013 at 09:53:36AM +0800, Aaron Lu wrote:
>>>> On 02/12/2013 12:21 AM, Seth Forshee wrote:
>>>>> The AML implementation for brightness control on several ThinkPads
>>>>> contains a workaround to meet a Windows 8 requirement of 101 brightness
>>>>> levels [1]. The implementation is flawed, as only 16 of the brighness
>>>>> values reported by _BCL affect a change in brightness. _BCM silently
>>>>> discards the rest of the values. Disabling Windows 8 compatibility on
>>>>> these machines reverts them to the old behavior, making _BCL only report
>>>>> the 16 brightness levels which actually work. Add a quirk to do this
>>>>> along with a dmi callback to disable Win8 compatibility.
>>>>
>>>> If we disable the _BQC(i.e. set cap._BQC=0) for these systems, will the
>>>> problem go away? If so, I think perhaps we can put these systems into a
>>>> _BQC quirk table and set cap._BQC=0 for them.
>>>
>>> That helps a little, but we're still left with only 16 of the 101
>>> brightness levels causing any change in brightness. The firmware isn't
>>> rounding the "bad" values or anything like that; it just silently
>>> ignores them.
>>
>> I really wondered, how Windows handled this, it should have the same
>> problem, unless they are not using the acpi video interface?
> 
> I can only guess.
> 
> I think I remember reading that Windows 8 does smooth backlight
> transitions, so it may well hit every intermediate brightness value.
> Lenovo could also be supplying a driver which rounds values to the
> nearest working value or uses some other interface or something else.

Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
access to at least 100 distinct brightness levels.

I'd guess it's using the same interface as
/sys/class/backlight/intel_backlight, which on my system has a
max_brightness of 4438 and all the values seem to be actually distinct,
if not necessarily discernible to the naked eye.

-Ben

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/03/2013 03:04 PM, Ben Jencks wrote:
> On 04/02/2013 09:00 AM, Seth Forshee wrote:
>> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
>>> On 04/01/2013 09:03 PM, Seth Forshee wrote:
>>>> On Mon, Apr 01, 2013 at 09:53:36AM +0800, Aaron Lu wrote:
>>>>> On 02/12/2013 12:21 AM, Seth Forshee wrote:
>>>>>> The AML implementation for brightness control on several ThinkPads
>>>>>> contains a workaround to meet a Windows 8 requirement of 101 brightness
>>>>>> levels [1]. The implementation is flawed, as only 16 of the brighness
>>>>>> values reported by _BCL affect a change in brightness. _BCM silently
>>>>>> discards the rest of the values. Disabling Windows 8 compatibility on
>>>>>> these machines reverts them to the old behavior, making _BCL only report
>>>>>> the 16 brightness levels which actually work. Add a quirk to do this
>>>>>> along with a dmi callback to disable Win8 compatibility.
>>>>>
>>>>> If we disable the _BQC(i.e. set cap._BQC=0) for these systems, will the
>>>>> problem go away? If so, I think perhaps we can put these systems into a
>>>>> _BQC quirk table and set cap._BQC=0 for them.
>>>>
>>>> That helps a little, but we're still left with only 16 of the 101
>>>> brightness levels causing any change in brightness. The firmware isn't
>>>> rounding the "bad" values or anything like that; it just silently
>>>> ignores them.
>>>
>>> I really wondered, how Windows handled this, it should have the same
>>> problem, unless they are not using the acpi video interface?
>>
>> I can only guess.
>>
>> I think I remember reading that Windows 8 does smooth backlight
>> transitions, so it may well hit every intermediate brightness value.
>> Lenovo could also be supplying a driver which rounds values to the
>> nearest working value or uses some other interface or something else.
> 
> Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
> access to at least 100 distinct brightness levels.
> 
> I'd guess it's using the same interface as
> /sys/class/backlight/intel_backlight, which on my system has a
> max_brightness of 4438 and all the values seem to be actually distinct,
> if not necessarily discernible to the naked eye.

Thanks for the information.

If all these affected systems have gpu driver's interface, I think we
can simply add them to the video_detect_dmi_table so that no acpi
backlight interface will be created for them, and gpu's interface will
be used by user space and hotkey processing can still be handled by acpi
video driver.

Best regards,
Aaron

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Wed, Apr 03, 2013 at 03:27:46PM +0800, Aaron Lu wrote:
> On 04/03/2013 03:04 PM, Ben Jencks wrote:
> > On 04/02/2013 09:00 AM, Seth Forshee wrote:
> >> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
> >>> On 04/01/2013 09:03 PM, Seth Forshee wrote:
> >>>> On Mon, Apr 01, 2013 at 09:53:36AM +0800, Aaron Lu wrote:
> >>>>> On 02/12/2013 12:21 AM, Seth Forshee wrote:
> >>>>>> The AML implementation for brightness control on several ThinkPads
> >>>>>> contains a workaround to meet a Windows 8 requirement of 101 brightness
> >>>>>> levels [1]. The implementation is flawed, as only 16 of the brighness
> >>>>>> values reported by _BCL affect a change in brightness. _BCM silently
> >>>>>> discards the rest of the values. Disabling Windows 8 compatibility on
> >>>>>> these machines reverts them to the old behavior, making _BCL only report
> >>>>>> the 16 brightness levels which actually work. Add a quirk to do this
> >>>>>> along with a dmi callback to disable Win8 compatibility.
> >>>>>
> >>>>> If we disable the _BQC(i.e. set cap._BQC=0) for these systems, will the
> >>>>> problem go away? If so, I think perhaps we can put these systems into a
> >>>>> _BQC quirk table and set cap._BQC=0 for them.
> >>>>
> >>>> That helps a little, but we're still left with only 16 of the 101
> >>>> brightness levels causing any change in brightness. The firmware isn't
> >>>> rounding the "bad" values or anything like that; it just silently
> >>>> ignores them.
> >>>
> >>> I really wondered, how Windows handled this, it should have the same
> >>> problem, unless they are not using the acpi video interface?
> >>
> >> I can only guess.
> >>
> >> I think I remember reading that Windows 8 does smooth backlight
> >> transitions, so it may well hit every intermediate brightness value.
> >> Lenovo could also be supplying a driver which rounds values to the
> >> nearest working value or uses some other interface or something else.
> > 
> > Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
> > access to at least 100 distinct brightness levels.
> > 
> > I'd guess it's using the same interface as
> > /sys/class/backlight/intel_backlight, which on my system has a
> > max_brightness of 4438 and all the values seem to be actually distinct,
> > if not necessarily discernible to the naked eye.
> 
> Thanks for the information.
> 
> If all these affected systems have gpu driver's interface, I think we
> can simply add them to the video_detect_dmi_table so that no acpi
> backlight interface will be created for them, and gpu's interface will
> be used by user space and hotkey processing can still be handled by acpi
> video driver.

I've got two machines here with discrete graphics, one with nvidia and
one with AMD (neither one is a Lenovo). With the nvidia machine neither
nouveau nor the proprietary driver registers a backlight device. On the
AMD machine radeon does register a backlight but the proprietary driver
does not. So I think quirking away the acpi_video backlight interface
has potential to leave some users without any backlight control at all.

Seth

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/03/2013 09:45 PM, Seth Forshee wrote:
> On Wed, Apr 03, 2013 at 03:27:46PM +0800, Aaron Lu wrote:
>> On 04/03/2013 03:04 PM, Ben Jencks wrote:
>>> On 04/02/2013 09:00 AM, Seth Forshee wrote:
>>>> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
>>>>> On 04/01/2013 09:03 PM, Seth Forshee wrote:
>>>>>> On Mon, Apr 01, 2013 at 09:53:36AM +0800, Aaron Lu wrote:
>>>>>>> On 02/12/2013 12:21 AM, Seth Forshee wrote:
>>>>>>>> The AML implementation for brightness control on several ThinkPads
>>>>>>>> contains a workaround to meet a Windows 8 requirement of 101 brightness
>>>>>>>> levels [1]. The implementation is flawed, as only 16 of the brighness
>>>>>>>> values reported by _BCL affect a change in brightness. _BCM silently
>>>>>>>> discards the rest of the values. Disabling Windows 8 compatibility on
>>>>>>>> these machines reverts them to the old behavior, making _BCL only report
>>>>>>>> the 16 brightness levels which actually work. Add a quirk to do this
>>>>>>>> along with a dmi callback to disable Win8 compatibility.
>>>>>>>
>>>>>>> If we disable the _BQC(i.e. set cap._BQC=0) for these systems, will the
>>>>>>> problem go away? If so, I think perhaps we can put these systems into a
>>>>>>> _BQC quirk table and set cap._BQC=0 for them.
>>>>>>
>>>>>> That helps a little, but we're still left with only 16 of the 101
>>>>>> brightness levels causing any change in brightness. The firmware isn't
>>>>>> rounding the "bad" values or anything like that; it just silently
>>>>>> ignores them.
>>>>>
>>>>> I really wondered, how Windows handled this, it should have the same
>>>>> problem, unless they are not using the acpi video interface?
>>>>
>>>> I can only guess.
>>>>
>>>> I think I remember reading that Windows 8 does smooth backlight
>>>> transitions, so it may well hit every intermediate brightness value.
>>>> Lenovo could also be supplying a driver which rounds values to the
>>>> nearest working value or uses some other interface or something else.
>>>
>>> Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
>>> access to at least 100 distinct brightness levels.
>>>
>>> I'd guess it's using the same interface as
>>> /sys/class/backlight/intel_backlight, which on my system has a
>>> max_brightness of 4438 and all the values seem to be actually distinct,
>>> if not necessarily discernible to the naked eye.
>>
>> Thanks for the information.
>>
>> If all these affected systems have gpu driver's interface, I think we
>> can simply add them to the video_detect_dmi_table so that no acpi
>> backlight interface will be created for them, and gpu's interface will
>> be used by user space and hotkey processing can still be handled by acpi
>> video driver.
> 
> I've got two machines here with discrete graphics, one with nvidia and
> one with AMD (neither one is a Lenovo). With the nvidia machine neither
> nouveau nor the proprietary driver registers a backlight device. On the
> AMD machine radeon does register a backlight but the proprietary driver
> does not. So I think quirking away the acpi_video backlight interface
> has potential to leave some users without any backlight control at all.

Then let's try to work around this problem in acpi video driver.

Thanks,
Aaron

> 
> Seth
> --
> 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 3/5] acpi_video: Add workaround for broken Windows 8 backlight implementations

[Aaron Lu]

On 03/08/2013 03:39 AM, Seth Forshee wrote:
> Windows 8 requires that all backlights report 101 brightness levels.
> When Lenovo updated the firmware for some machines for Windows 8 they
> met this requirement my making _BCL return a larger set of values for
> Windows 8 than for other OSes. However, only the values in the smaller
> set actually change the brightness at all. The rest of the values are
> silently discarded.
> 
> As a workaround, change acpi_video to set all intermediate backlight
> levels when setting the brightness. This isn't perfect, but it will mean
> that most brightness changes done by common userspace utilities will hit
> at least one valid brightness value.
> 
> [1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx
> 
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  drivers/acpi/video.c |   51 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index edfcd74..b83fbbd 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -352,25 +352,56 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
>  static int
>  acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state)
>  {
> -	int level = device->brightness->levels[state];
>  	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
>  	struct acpi_object_list args = { 1, &arg0 };
> +	int curr_state, offset;
>  	acpi_status status;
> +	int result = 0;
>  
> -	arg0.integer.value = level;
> +	curr_state = device->brightness->curr_state;
>  
> -	status = acpi_evaluate_object(device->dev->handle, "_BCM",
> -				      &args, NULL);
> -	if (ACPI_FAILURE(status)) {
> -		ACPI_ERROR((AE_INFO, "Evaluating _BCM failed"));
> -		return -EIO;
> +	/*
> +	 * Some Lenovo firmware has a broken backlight implementation
> +	 * for Windows 8 where _BCL returns 101 backlight levels but
> +	 * only 16 or so levels actually change the brightness at all.
> +	 * As a workaround for these machines we set every intermediate
> +	 * value between the old and new brightness levels whenever the
> +	 * system has made the Windows 8 OSI call, hoping that at least
> +	 * one of them will cause a change in brightness.
> +	 */
> +	if (acpi_osi_windows_version() == ACPI_OSI_WIN_8) {

What do you think of testing br->count > 100 instead of OSI version? It
looks like only win8 systems will try to claim so many brightness levels.

Thanks,
Aaron

> +		if (state == curr_state)
> +			offset = 0;
> +		else
> +			offset = state > curr_state ? 1 : -1;
> +	} else {
> +		offset = state - curr_state;
>  	}
>  
> -	device->brightness->curr_state = state;
> +	do {
> +		curr_state += offset;
> +		arg0.integer.value = device->brightness->levels[curr_state];
> +
> +		status = acpi_evaluate_object(device->dev->handle, "_BCM",
> +					      &args, NULL);
> +		if (ACPI_FAILURE(status)) {
> +			ACPI_ERROR((AE_INFO, "Evaluating _BCM failed"));
> +
> +			/*
> +			 * Change curr_state back to that of last
> +			 * successful _BCM call
> +			 */
> +			curr_state -= offset;
> +			result = -EIO;
> +			break;
> +		}
> +	} while (curr_state != state);
> +
> +	device->brightness->curr_state = curr_state;
>  	if (device->backlight)
> -		device->backlight->props.brightness = state - 2;
> +		device->backlight->props.brightness = curr_state - 2;
>  
> -	return 0;
> +	return result;
>  }
>  
>  static int
> 

Re: [PATCH 3/5] acpi_video: Add workaround for broken Windows 8 backlight implementations

[Seth Forshee]

On Thu, Apr 04, 2013 at 07:44:04PM +0800, Aaron Lu wrote:
> On 03/08/2013 03:39 AM, Seth Forshee wrote:
> > Windows 8 requires that all backlights report 101 brightness levels.
> > When Lenovo updated the firmware for some machines for Windows 8 they
> > met this requirement my making _BCL return a larger set of values for
> > Windows 8 than for other OSes. However, only the values in the smaller
> > set actually change the brightness at all. The rest of the values are
> > silently discarded.
> > 
> > As a workaround, change acpi_video to set all intermediate backlight
> > levels when setting the brightness. This isn't perfect, but it will mean
> > that most brightness changes done by common userspace utilities will hit
> > at least one valid brightness value.
> > 
> > [1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx
> > 
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> >  drivers/acpi/video.c |   51 ++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 41 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> > index edfcd74..b83fbbd 100644
> > --- a/drivers/acpi/video.c
> > +++ b/drivers/acpi/video.c
> > @@ -352,25 +352,56 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
> >  static int
> >  acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state)
> >  {
> > -	int level = device->brightness->levels[state];
> >  	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> >  	struct acpi_object_list args = { 1, &arg0 };
> > +	int curr_state, offset;
> >  	acpi_status status;
> > +	int result = 0;
> >  
> > -	arg0.integer.value = level;
> > +	curr_state = device->brightness->curr_state;
> >  
> > -	status = acpi_evaluate_object(device->dev->handle, "_BCM",
> > -				      &args, NULL);
> > -	if (ACPI_FAILURE(status)) {
> > -		ACPI_ERROR((AE_INFO, "Evaluating _BCM failed"));
> > -		return -EIO;
> > +	/*
> > +	 * Some Lenovo firmware has a broken backlight implementation
> > +	 * for Windows 8 where _BCL returns 101 backlight levels but
> > +	 * only 16 or so levels actually change the brightness at all.
> > +	 * As a workaround for these machines we set every intermediate
> > +	 * value between the old and new brightness levels whenever the
> > +	 * system has made the Windows 8 OSI call, hoping that at least
> > +	 * one of them will cause a change in brightness.
> > +	 */
> > +	if (acpi_osi_windows_version() == ACPI_OSI_WIN_8) {
> 
> What do you think of testing br->count > 100 instead of OSI version? It
> looks like only win8 systems will try to claim so many brightness levels.

I agree that it would be roughly the same set of machines today. But if
we assume Microsoft will keep the same requirement in the future then it
begins to expand beyond Windows 8.

If anything I'd prefer reducing the number of machines we apply this
workaround to. Like say limiting it to Lenovo Win8 machines, if we can
reasonably assume that Lenovo will be the only vendor with this
ridiculous implementation.

Seth

Re: [PATCH 3/5] acpi_video: Add workaround for broken Windows 8 backlight implementations

[Aaron Lu]

On 04/04/2013 08:35 PM, Seth Forshee wrote:
> On Thu, Apr 04, 2013 at 07:44:04PM +0800, Aaron Lu wrote:
>> On 03/08/2013 03:39 AM, Seth Forshee wrote:
>>> Windows 8 requires that all backlights report 101 brightness levels.
>>> When Lenovo updated the firmware for some machines for Windows 8 they
>>> met this requirement my making _BCL return a larger set of values for
>>> Windows 8 than for other OSes. However, only the values in the smaller
>>> set actually change the brightness at all. The rest of the values are
>>> silently discarded.
>>>
>>> As a workaround, change acpi_video to set all intermediate backlight
>>> levels when setting the brightness. This isn't perfect, but it will mean
>>> that most brightness changes done by common userspace utilities will hit
>>> at least one valid brightness value.
>>>
>>> [1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx
>>>
>>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>>> ---
>>>  drivers/acpi/video.c |   51 ++++++++++++++++++++++++++++++++++++++++----------
>>>  1 file changed, 41 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>>> index edfcd74..b83fbbd 100644
>>> --- a/drivers/acpi/video.c
>>> +++ b/drivers/acpi/video.c
>>> @@ -352,25 +352,56 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
>>>  static int
>>>  acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state)
>>>  {
>>> -	int level = device->brightness->levels[state];
>>>  	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
>>>  	struct acpi_object_list args = { 1, &arg0 };
>>> +	int curr_state, offset;
>>>  	acpi_status status;
>>> +	int result = 0;
>>>  
>>> -	arg0.integer.value = level;
>>> +	curr_state = device->brightness->curr_state;
>>>  
>>> -	status = acpi_evaluate_object(device->dev->handle, "_BCM",
>>> -				      &args, NULL);
>>> -	if (ACPI_FAILURE(status)) {
>>> -		ACPI_ERROR((AE_INFO, "Evaluating _BCM failed"));
>>> -		return -EIO;
>>> +	/*
>>> +	 * Some Lenovo firmware has a broken backlight implementation
>>> +	 * for Windows 8 where _BCL returns 101 backlight levels but
>>> +	 * only 16 or so levels actually change the brightness at all.
>>> +	 * As a workaround for these machines we set every intermediate
>>> +	 * value between the old and new brightness levels whenever the
>>> +	 * system has made the Windows 8 OSI call, hoping that at least
>>> +	 * one of them will cause a change in brightness.
>>> +	 */
>>> +	if (acpi_osi_windows_version() == ACPI_OSI_WIN_8) {
>>
>> What do you think of testing br->count > 100 instead of OSI version? It
>> looks like only win8 systems will try to claim so many brightness levels.
> 
> I agree that it would be roughly the same set of machines today. But if
> we assume Microsoft will keep the same requirement in the future then it
> begins to expand beyond Windows 8.

Right, and the br->count > 100 test should also work, so it seems to be
a better condition check than OSI version.

> 
> If anything I'd prefer reducing the number of machines we apply this
> workaround to. Like say limiting it to Lenovo Win8 machines, if we can
> reasonably assume that Lenovo will be the only vendor with this
> ridiculous implementation.

This is probably not the case. I saw a Dell system also claims to have
100 levels in win8 mode:
https://bugzilla.kernel.org/show_bug.cgi?id=55071

Thanks,
Aaron

Re: [PATCH 3/5] acpi_video: Add workaround for broken Windows 8 backlight implementations

[Seth Forshee]

On Thu, Apr 04, 2013 at 09:46:47PM +0800, Aaron Lu wrote:
> On 04/04/2013 08:35 PM, Seth Forshee wrote:
> > On Thu, Apr 04, 2013 at 07:44:04PM +0800, Aaron Lu wrote:
> >> On 03/08/2013 03:39 AM, Seth Forshee wrote:
> >>> Windows 8 requires that all backlights report 101 brightness levels.
> >>> When Lenovo updated the firmware for some machines for Windows 8 they
> >>> met this requirement my making _BCL return a larger set of values for
> >>> Windows 8 than for other OSes. However, only the values in the smaller
> >>> set actually change the brightness at all. The rest of the values are
> >>> silently discarded.
> >>>
> >>> As a workaround, change acpi_video to set all intermediate backlight
> >>> levels when setting the brightness. This isn't perfect, but it will mean
> >>> that most brightness changes done by common userspace utilities will hit
> >>> at least one valid brightness value.
> >>>
> >>> [1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx
> >>>
> >>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> >>> ---
> >>>  drivers/acpi/video.c |   51 ++++++++++++++++++++++++++++++++++++++++----------
> >>>  1 file changed, 41 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> >>> index edfcd74..b83fbbd 100644
> >>> --- a/drivers/acpi/video.c
> >>> +++ b/drivers/acpi/video.c
> >>> @@ -352,25 +352,56 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
> >>>  static int
> >>>  acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state)
> >>>  {
> >>> -	int level = device->brightness->levels[state];
> >>>  	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> >>>  	struct acpi_object_list args = { 1, &arg0 };
> >>> +	int curr_state, offset;
> >>>  	acpi_status status;
> >>> +	int result = 0;
> >>>  
> >>> -	arg0.integer.value = level;
> >>> +	curr_state = device->brightness->curr_state;
> >>>  
> >>> -	status = acpi_evaluate_object(device->dev->handle, "_BCM",
> >>> -				      &args, NULL);
> >>> -	if (ACPI_FAILURE(status)) {
> >>> -		ACPI_ERROR((AE_INFO, "Evaluating _BCM failed"));
> >>> -		return -EIO;
> >>> +	/*
> >>> +	 * Some Lenovo firmware has a broken backlight implementation
> >>> +	 * for Windows 8 where _BCL returns 101 backlight levels but
> >>> +	 * only 16 or so levels actually change the brightness at all.
> >>> +	 * As a workaround for these machines we set every intermediate
> >>> +	 * value between the old and new brightness levels whenever the
> >>> +	 * system has made the Windows 8 OSI call, hoping that at least
> >>> +	 * one of them will cause a change in brightness.
> >>> +	 */
> >>> +	if (acpi_osi_windows_version() == ACPI_OSI_WIN_8) {
> >>
> >> What do you think of testing br->count > 100 instead of OSI version? It
> >> looks like only win8 systems will try to claim so many brightness levels.
> > 
> > I agree that it would be roughly the same set of machines today. But if
> > we assume Microsoft will keep the same requirement in the future then it
> > begins to expand beyond Windows 8.
> 
> Right, and the br->count > 100 test should also work, so it seems to be
> a better condition check than OSI version.

My take on this is that for Lenovo this is an issue of transitioning to
Windows 8. As far as I can tell the affected machines were all sold with
Windows 7 previously and updated for Windows 8, and the implementation
we're seeing looks like it's just a lazy way to meet the 101 brightness
levels requirement. If that's true then extending it past Windows 8
doesn't make sense. Unfortunately I haven't been able to get Lenovo to
comment on it, so right now it's just a guess.

> > If anything I'd prefer reducing the number of machines we apply this
> > workaround to. Like say limiting it to Lenovo Win8 machines, if we can
> > reasonably assume that Lenovo will be the only vendor with this
> > ridiculous implementation.
> 
> This is probably not the case. I saw a Dell system also claims to have
> 100 levels in win8 mode:
> https://bugzilla.kernel.org/show_bug.cgi?id=55071

For that bug it looks like even writing the !Win8 values doesn't change
the brightness, so this workaround isn't going to help anyway.

Seth

Re: [PATCH 3/5] acpi_video: Add workaround for broken Windows 8 backlight implementations

[Aaron Lu]

On 04/04/2013 10:02 PM, Seth Forshee wrote:
> On Thu, Apr 04, 2013 at 09:46:47PM +0800, Aaron Lu wrote:
>> On 04/04/2013 08:35 PM, Seth Forshee wrote:
>>> On Thu, Apr 04, 2013 at 07:44:04PM +0800, Aaron Lu wrote:
>>>> On 03/08/2013 03:39 AM, Seth Forshee wrote:
>>>>> Windows 8 requires that all backlights report 101 brightness levels.
>>>>> When Lenovo updated the firmware for some machines for Windows 8 they
>>>>> met this requirement my making _BCL return a larger set of values for
>>>>> Windows 8 than for other OSes. However, only the values in the smaller
>>>>> set actually change the brightness at all. The rest of the values are
>>>>> silently discarded.
>>>>>
>>>>> As a workaround, change acpi_video to set all intermediate backlight
>>>>> levels when setting the brightness. This isn't perfect, but it will mean
>>>>> that most brightness changes done by common userspace utilities will hit
>>>>> at least one valid brightness value.
>>>>>
>>>>> [1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx
>>>>>
>>>>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>>>>> ---
>>>>>  drivers/acpi/video.c |   51 ++++++++++++++++++++++++++++++++++++++++----------
>>>>>  1 file changed, 41 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>>>>> index edfcd74..b83fbbd 100644
>>>>> --- a/drivers/acpi/video.c
>>>>> +++ b/drivers/acpi/video.c
>>>>> @@ -352,25 +352,56 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
>>>>>  static int
>>>>>  acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state)
>>>>>  {
>>>>> -	int level = device->brightness->levels[state];
>>>>>  	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
>>>>>  	struct acpi_object_list args = { 1, &arg0 };
>>>>> +	int curr_state, offset;
>>>>>  	acpi_status status;
>>>>> +	int result = 0;
>>>>>  
>>>>> -	arg0.integer.value = level;
>>>>> +	curr_state = device->brightness->curr_state;
>>>>>  
>>>>> -	status = acpi_evaluate_object(device->dev->handle, "_BCM",
>>>>> -				      &args, NULL);
>>>>> -	if (ACPI_FAILURE(status)) {
>>>>> -		ACPI_ERROR((AE_INFO, "Evaluating _BCM failed"));
>>>>> -		return -EIO;
>>>>> +	/*
>>>>> +	 * Some Lenovo firmware has a broken backlight implementation
>>>>> +	 * for Windows 8 where _BCL returns 101 backlight levels but
>>>>> +	 * only 16 or so levels actually change the brightness at all.
>>>>> +	 * As a workaround for these machines we set every intermediate
>>>>> +	 * value between the old and new brightness levels whenever the
>>>>> +	 * system has made the Windows 8 OSI call, hoping that at least
>>>>> +	 * one of them will cause a change in brightness.
>>>>> +	 */
>>>>> +	if (acpi_osi_windows_version() == ACPI_OSI_WIN_8) {
>>>>
>>>> What do you think of testing br->count > 100 instead of OSI version? It
>>>> looks like only win8 systems will try to claim so many brightness levels.
>>>
>>> I agree that it would be roughly the same set of machines today. But if
>>> we assume Microsoft will keep the same requirement in the future then it
>>> begins to expand beyond Windows 8.
>>
>> Right, and the br->count > 100 test should also work, so it seems to be
>> a better condition check than OSI version.
> 
> My take on this is that for Lenovo this is an issue of transitioning to
> Windows 8. As far as I can tell the affected machines were all sold with
> Windows 7 previously and updated for Windows 8, and the implementation
> we're seeing looks like it's just a lazy way to meet the 101 brightness
> levels requirement. If that's true then extending it past Windows 8
> doesn't make sense. Unfortunately I haven't been able to get Lenovo to
> comment on it, so right now it's just a guess.

Then perhaps use a DMI table for them?

> 
>>> If anything I'd prefer reducing the number of machines we apply this
>>> workaround to. Like say limiting it to Lenovo Win8 machines, if we can
>>> reasonably assume that Lenovo will be the only vendor with this
>>> ridiculous implementation.
>>
>> This is probably not the case. I saw a Dell system also claims to have
>> 100 levels in win8 mode:
>> https://bugzilla.kernel.org/show_bug.cgi?id=55071
> 
> For that bug it looks like even writing the !Win8 values doesn't change
> the brightness, so this workaround isn't going to help anyway.

Oh, I thought you mean 101 levels, but obviously you mean the ridiculous
implementations.

-Aaron

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/03/2013 03:04 PM, Ben Jencks wrote:
> On 04/02/2013 09:00 AM, Seth Forshee wrote:
>> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
>>>
>>> I really wondered, how Windows handled this, it should have the same
>>> problem, unless they are not using the acpi video interface?
>>
>> I can only guess.
>>
>> I think I remember reading that Windows 8 does smooth backlight
>> transitions, so it may well hit every intermediate brightness value.
>> Lenovo could also be supplying a driver which rounds values to the
>> nearest working value or uses some other interface or something else.
> 
> Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
> access to at least 100 distinct brightness levels.

I just came across a document on win8 backlight control, it has words
like this:
"
In Windows 8, the primary mechanism by which a platform should expose
its display brightness control functionality is the Windows Display
Driver Model (WDDM) miniport Device Driver Interfaces (DDI).
"
So looks like, on win8, ACPI interface is not used for these systems.

The link for the document is here:
http://msdn.microsoft.com/en-US/library/windows/hardware/jj159305

-Aaron

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Mon, Mar 18, 2013 at 04:25:46PM -0500, Seth Forshee wrote:
> > I've had the patches implementing your suggestions written for a while
> > now but just got test feedback this week. So here they are.
> 
> Ping.
> 
> Matthew, do these patches look like what you had in mind?

What needs to be done to move forward on this issue? Because so far no
one besides Aaron has made any comment at all on the actual content of
the patches.

Thanks,
Seth

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Rafael J. Wysocki]

On Friday, April 19, 2013 07:24:03 AM Seth Forshee wrote:
> On Mon, Mar 18, 2013 at 04:25:46PM -0500, Seth Forshee wrote:
> > > I've had the patches implementing your suggestions written for a while
> > > now but just got test feedback this week. So here they are.
> > 
> > Ping.
> > 
> > Matthew, do these patches look like what you had in mind?
> 
> What needs to be done to move forward on this issue? Because so far no
> one besides Aaron has made any comment at all on the actual content of
> the patches.

I'm not commenting mostly because I haven't had the time to think more about
this issue yet.  Sorry about that.

If I were to comment without much consideration, though, I'd say I wasn't
particularly impressed by any approach mentioned so far.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Rafael J. Wysocki]

On Friday, April 19, 2013 11:15:57 AM Aaron Lu wrote:
> On 04/03/2013 03:04 PM, Ben Jencks wrote:
> > On 04/02/2013 09:00 AM, Seth Forshee wrote:
> >> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
> >>>
> >>> I really wondered, how Windows handled this, it should have the same
> >>> problem, unless they are not using the acpi video interface?
> >>
> >> I can only guess.
> >>
> >> I think I remember reading that Windows 8 does smooth backlight
> >> transitions, so it may well hit every intermediate brightness value.
> >> Lenovo could also be supplying a driver which rounds values to the
> >> nearest working value or uses some other interface or something else.
> > 
> > Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
> > access to at least 100 distinct brightness levels.
> 
> I just came across a document on win8 backlight control, it has words
> like this:
> "
> In Windows 8, the primary mechanism by which a platform should expose
> its display brightness control functionality is the Windows Display
> Driver Model (WDDM) miniport Device Driver Interfaces (DDI).
> "
> So looks like, on win8, ACPI interface is not used for these systems.
> 
> The link for the document is here:
> http://msdn.microsoft.com/en-US/library/windows/hardware/jj159305

OK, so what does that mean for the issue at hand?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Sun, Apr 21, 2013 at 12:06:00AM +0200, Rafael J. Wysocki wrote:
> On Friday, April 19, 2013 07:24:03 AM Seth Forshee wrote:
> > On Mon, Mar 18, 2013 at 04:25:46PM -0500, Seth Forshee wrote:
> > > > I've had the patches implementing your suggestions written for a while
> > > > now but just got test feedback this week. So here they are.
> > > 
> > > Ping.
> > > 
> > > Matthew, do these patches look like what you had in mind?
> > 
> > What needs to be done to move forward on this issue? Because so far no
> > one besides Aaron has made any comment at all on the actual content of
> > the patches.
> 
> I'm not commenting mostly because I haven't had the time to think more about
> this issue yet.  Sorry about that.
> 
> If I were to comment without much consideration, though, I'd say I wasn't
> particularly impressed by any approach mentioned so far.

I doubt there's going to be any "good" solution to this problem. What
gives the best results is to force the affected models back to the
pre-Win8 behavior, but of course that requires quirking. If we could
assume that all graphics drivers will export backlight control then just
disabling the ACPI interface for these machines would work nearly as
well, but I don't think we can count on that, and it probably requires
quirking as well.

If we rule out quirking there are only a couple of options that anyone
has suggested. We can hit every intermediate brightness level as in the
patches I sent, but this is imperfect and requires a number of
comprimises. Or we can test every level the machine claims to support
and see which ones cause the value returned by _BQC to change, which
seems like it ought to work but means changing the brightness during
boot (which Matthew already objected to, though in another context). I
haven't been able to think of any others, but I'm happy to listen to
suggestions.

Thanks,
Seth

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/21/2013 06:06 AM, Rafael J. Wysocki wrote:
> On Friday, April 19, 2013 11:15:57 AM Aaron Lu wrote:
>> On 04/03/2013 03:04 PM, Ben Jencks wrote:
>>> On 04/02/2013 09:00 AM, Seth Forshee wrote:
>>>> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
>>>>>
>>>>> I really wondered, how Windows handled this, it should have the same
>>>>> problem, unless they are not using the acpi video interface?
>>>>
>>>> I can only guess.
>>>>
>>>> I think I remember reading that Windows 8 does smooth backlight
>>>> transitions, so it may well hit every intermediate brightness value.
>>>> Lenovo could also be supplying a driver which rounds values to the
>>>> nearest working value or uses some other interface or something else.
>>>
>>> Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
>>> access to at least 100 distinct brightness levels.
>>
>> I just came across a document on win8 backlight control, it has words
>> like this:
>> "
>> In Windows 8, the primary mechanism by which a platform should expose
>> its display brightness control functionality is the Windows Display
>> Driver Model (WDDM) miniport Device Driver Interfaces (DDI).
>> "
>> So looks like, on win8, ACPI interface is not used for these systems.
>>
>> The link for the document is here:
>> http://msdn.microsoft.com/en-US/library/windows/hardware/jj159305
>
> OK, so what does that mean for the issue at hand?

That means, we should not try to use acpi video interface to control
backlight on these systems if they are in win8 mode.

-Aaron

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/21/2013 07:07 PM, Aaron Lu wrote:
> On 04/21/2013 06:06 AM, Rafael J. Wysocki wrote:
>> On Friday, April 19, 2013 11:15:57 AM Aaron Lu wrote:
>>> On 04/03/2013 03:04 PM, Ben Jencks wrote:
>>>> On 04/02/2013 09:00 AM, Seth Forshee wrote:
>>>>> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
>>>>>>
>>>>>> I really wondered, how Windows handled this, it should have the same
>>>>>> problem, unless they are not using the acpi video interface?
>>>>>
>>>>> I can only guess.
>>>>>
>>>>> I think I remember reading that Windows 8 does smooth backlight
>>>>> transitions, so it may well hit every intermediate brightness value.
>>>>> Lenovo could also be supplying a driver which rounds values to the
>>>>> nearest working value or uses some other interface or something else.
>>>>
>>>> Just checked; Windows 8 doesn't use the ACPI interface. It seems to
>>>> have
>>>> access to at least 100 distinct brightness levels.
>>>
>>> I just came across a document on win8 backlight control, it has words
>>> like this:
>>> "
>>> In Windows 8, the primary mechanism by which a platform should expose
>>> its display brightness control functionality is the Windows Display
>>> Driver Model (WDDM) miniport Device Driver Interfaces (DDI).
>>> "
>>> So looks like, on win8, ACPI interface is not used for these systems.
>>>
>>> The link for the document is here:
>>> http://msdn.microsoft.com/en-US/library/windows/hardware/jj159305
>>
>> OK, so what does that mean for the issue at hand?
>
> That means, we should not try to use acpi video interface to control
> backlight on these systems if they are in win8 mode.

The reason I mentioned this document is: we have been wondering how
windows handle such systems, and according to Ben's own use of win8, he
thinks win8 is not using acpi interface to control backlight. And the
words from the document that I pasted here kind of confirms this I think,
so I mentioned this document here, it is used to confirm how windows
handle such systems: using a native driver instead of acpi video
interface.

Thanks,
Aaron

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Henrique de Moraes Holschuh]

On Sat, 20 Apr 2013, Seth Forshee wrote:
> pre-Win8 behavior, but of course that requires quirking. If we could
> assume that all graphics drivers will export backlight control then just
> disabling the ACPI interface for these machines would work nearly as
> well, but I don't think we can count on that, and it probably requires
> quirking as well.

You can certainly do that for all thinkpads with win8 compatibility.

-- 
  "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] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Rafael J. Wysocki]

On Sunday, April 21, 2013 07:07:04 PM Aaron Lu wrote:
> On 04/21/2013 06:06 AM, Rafael J. Wysocki wrote:
> > On Friday, April 19, 2013 11:15:57 AM Aaron Lu wrote:
> >> On 04/03/2013 03:04 PM, Ben Jencks wrote:
> >>> On 04/02/2013 09:00 AM, Seth Forshee wrote:
> >>>> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
> >>>>>
> >>>>> I really wondered, how Windows handled this, it should have the same
> >>>>> problem, unless they are not using the acpi video interface?
> >>>>
> >>>> I can only guess.
> >>>>
> >>>> I think I remember reading that Windows 8 does smooth backlight
> >>>> transitions, so it may well hit every intermediate brightness value.
> >>>> Lenovo could also be supplying a driver which rounds values to the
> >>>> nearest working value or uses some other interface or something else.
> >>>
> >>> Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
> >>> access to at least 100 distinct brightness levels.
> >>
> >> I just came across a document on win8 backlight control, it has words
> >> like this:
> >> "
> >> In Windows 8, the primary mechanism by which a platform should expose
> >> its display brightness control functionality is the Windows Display
> >> Driver Model (WDDM) miniport Device Driver Interfaces (DDI).
> >> "
> >> So looks like, on win8, ACPI interface is not used for these systems.
> >>
> >> The link for the document is here:
> >> http://msdn.microsoft.com/en-US/library/windows/hardware/jj159305
> >
> > OK, so what does that mean for the issue at hand?
> 
> That means, we should not try to use acpi video interface to control
> backlight on these systems if they are in win8 mode.

In that case, how are we going to indentify "these systems"?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[joeyli]

於 五，2013-04-19 於 11:15 +0800，Aaron Lu 提到：
> On 04/03/2013 03:04 PM, Ben Jencks wrote:
> > On 04/02/2013 09:00 AM, Seth Forshee wrote:
> >> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
> >>>
> >>> I really wondered, how Windows handled this, it should have the same
> >>> problem, unless they are not using the acpi video interface?
> >>
> >> I can only guess.
> >>
> >> I think I remember reading that Windows 8 does smooth backlight
> >> transitions, so it may well hit every intermediate brightness value.
> >> Lenovo could also be supplying a driver which rounds values to the
> >> nearest working value or uses some other interface or something else.
> > 
> > Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
> > access to at least 100 distinct brightness levels.
> 
> I just came across a document on win8 backlight control, it has words
> like this:
> "
> In Windows 8, the primary mechanism by which a platform should expose
> its display brightness control functionality is the Windows Display
> Driver Model (WDDM) miniport Device Driver Interfaces (DDI).
> "
> So looks like, on win8, ACPI interface is not used for these systems.
> 
> The link for the document is here:
> http://msdn.microsoft.com/en-US/library/windows/hardware/jj159305
> 
> -Aaron
> 

Per WDDM document, OEM/ODM should keep the ACPI methods (_BQL, _BCM,
_BQC) available for compliant to the OS that doesn't support WDDM, e.g.
XP, Vista.

But, I discussed with Acer for this situation at last year, they didn't
force their ODM do test ACPI compatibility. They only test through
Windows 8 UI with Fn keys.


Thanks a lot!
Joey Lee

--
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 Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/22/2013 05:42 AM, Rafael J. Wysocki wrote:
> On Sunday, April 21, 2013 07:07:04 PM Aaron Lu wrote:
>> On 04/21/2013 06:06 AM, Rafael J. Wysocki wrote:
>>> On Friday, April 19, 2013 11:15:57 AM Aaron Lu wrote:
>>>> On 04/03/2013 03:04 PM, Ben Jencks wrote:
>>>>> On 04/02/2013 09:00 AM, Seth Forshee wrote:
>>>>>> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
>>>>>>>
>>>>>>> I really wondered, how Windows handled this, it should have the same
>>>>>>> problem, unless they are not using the acpi video interface?
>>>>>>
>>>>>> I can only guess.
>>>>>>
>>>>>> I think I remember reading that Windows 8 does smooth backlight
>>>>>> transitions, so it may well hit every intermediate brightness value.
>>>>>> Lenovo could also be supplying a driver which rounds values to the
>>>>>> nearest working value or uses some other interface or something else.
>>>>>
>>>>> Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
>>>>> access to at least 100 distinct brightness levels.
>>>>
>>>> I just came across a document on win8 backlight control, it has words
>>>> like this:
>>>> "
>>>> In Windows 8, the primary mechanism by which a platform should expose
>>>> its display brightness control functionality is the Windows Display
>>>> Driver Model (WDDM) miniport Device Driver Interfaces (DDI).
>>>> "
>>>> So looks like, on win8, ACPI interface is not used for these systems.
>>>>
>>>> The link for the document is here:
>>>> http://msdn.microsoft.com/en-US/library/windows/hardware/jj159305
>>>
>>> OK, so what does that mean for the issue at hand?
>>
>> That means, we should not try to use acpi video interface to control
>> backlight on these systems if they are in win8 mode.
> 
> In that case, how are we going to indentify "these systems"?

Sorry, I don't know.

According to the following bug page:
https://bugzilla.kernel.org/show_bug.cgi?id=51231
"these systems" now include: thinkpad x230, t430s, t530, L430.

-Aaron

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/22/2013 10:18 AM, joeyli wrote:
> 於 五，2013-04-19 於 11:15 +0800，Aaron Lu 提到：
>> On 04/03/2013 03:04 PM, Ben Jencks wrote:
>>> On 04/02/2013 09:00 AM, Seth Forshee wrote:
>>>> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
>>>>>
>>>>> I really wondered, how Windows handled this, it should have the same
>>>>> problem, unless they are not using the acpi video interface?
>>>>
>>>> I can only guess.
>>>>
>>>> I think I remember reading that Windows 8 does smooth backlight
>>>> transitions, so it may well hit every intermediate brightness value.
>>>> Lenovo could also be supplying a driver which rounds values to the
>>>> nearest working value or uses some other interface or something else.
>>>
>>> Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
>>> access to at least 100 distinct brightness levels.
>>
>> I just came across a document on win8 backlight control, it has words
>> like this:
>> "
>> In Windows 8, the primary mechanism by which a platform should expose
>> its display brightness control functionality is the Windows Display
>> Driver Model (WDDM) miniport Device Driver Interfaces (DDI).
>> "
>> So looks like, on win8, ACPI interface is not used for these systems.
>>
>> The link for the document is here:
>> http://msdn.microsoft.com/en-US/library/windows/hardware/jj159305
>>
>> -Aaron
>>
> 
> Per WDDM document, OEM/ODM should keep the ACPI methods (_BQL, _BCM,
> _BQC) available for compliant to the OS that doesn't support WDDM, e.g.
> XP, Vista.

Thanks for the information, so this suggests that acpi interface is
mostly used for pre-win8 OSes?(does win7 support WDDM?)

One interesting thing is, if they do not support acpi interface, why
they even bother to expose this interface in acpi table?

-Aaron
--
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 Windows 8 compatibility for some Lenovo ThinkPads

[Rafael J. Wysocki]

On Monday, April 22, 2013 05:39:43 PM Aaron Lu wrote:
> On 04/22/2013 05:42 AM, Rafael J. Wysocki wrote:
> > On Sunday, April 21, 2013 07:07:04 PM Aaron Lu wrote:
> >> On 04/21/2013 06:06 AM, Rafael J. Wysocki wrote:
> >>> On Friday, April 19, 2013 11:15:57 AM Aaron Lu wrote:
> >>>> On 04/03/2013 03:04 PM, Ben Jencks wrote:
> >>>>> On 04/02/2013 09:00 AM, Seth Forshee wrote:
> >>>>>> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
> >>>>>>>
> >>>>>>> I really wondered, how Windows handled this, it should have the same
> >>>>>>> problem, unless they are not using the acpi video interface?
> >>>>>>
> >>>>>> I can only guess.
> >>>>>>
> >>>>>> I think I remember reading that Windows 8 does smooth backlight
> >>>>>> transitions, so it may well hit every intermediate brightness value.
> >>>>>> Lenovo could also be supplying a driver which rounds values to the
> >>>>>> nearest working value or uses some other interface or something else.
> >>>>>
> >>>>> Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
> >>>>> access to at least 100 distinct brightness levels.
> >>>>
> >>>> I just came across a document on win8 backlight control, it has words
> >>>> like this:
> >>>> "
> >>>> In Windows 8, the primary mechanism by which a platform should expose
> >>>> its display brightness control functionality is the Windows Display
> >>>> Driver Model (WDDM) miniport Device Driver Interfaces (DDI).
> >>>> "
> >>>> So looks like, on win8, ACPI interface is not used for these systems.
> >>>>
> >>>> The link for the document is here:
> >>>> http://msdn.microsoft.com/en-US/library/windows/hardware/jj159305
> >>>
> >>> OK, so what does that mean for the issue at hand?
> >>
> >> That means, we should not try to use acpi video interface to control
> >> backlight on these systems if they are in win8 mode.
> > 
> > In that case, how are we going to indentify "these systems"?
> 
> Sorry, I don't know.
> 
> According to the following bug page:
> https://bugzilla.kernel.org/show_bug.cgi?id=51231
> "these systems" now include: thinkpad x230, t430s, t530, L430.

I suppose we need to blacklist them to start with.  I wouldn't like to apply
any general changes before we know how many different systems are affected by
this particular issue.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[joeyli]

於 一，2013-04-22 於 18:08 +0800，Aaron Lu 提到：
> On 04/22/2013 10:18 AM, joeyli wrote:
> > 於 五，2013-04-19 於 11:15 +0800，Aaron Lu 提到：
> >> On 04/03/2013 03:04 PM, Ben Jencks wrote:
> >>> On 04/02/2013 09:00 AM, Seth Forshee wrote:
> >>>> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
> >>>>>
> >>>>> I really wondered, how Windows handled this, it should have the same
> >>>>> problem, unless they are not using the acpi video interface?
> >>>>
> >>>> I can only guess.
> >>>>
> >>>> I think I remember reading that Windows 8 does smooth backlight
> >>>> transitions, so it may well hit every intermediate brightness value.
> >>>> Lenovo could also be supplying a driver which rounds values to the
> >>>> nearest working value or uses some other interface or something else.
> >>>
> >>> Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
> >>> access to at least 100 distinct brightness levels.
> >>
> >> I just came across a document on win8 backlight control, it has words
> >> like this:
> >> "
> >> In Windows 8, the primary mechanism by which a platform should expose
> >> its display brightness control functionality is the Windows Display
> >> Driver Model (WDDM) miniport Device Driver Interfaces (DDI).
> >> "
> >> So looks like, on win8, ACPI interface is not used for these systems.
> >>
> >> The link for the document is here:
> >> http://msdn.microsoft.com/en-US/library/windows/hardware/jj159305
> >>
> >> -Aaron
> >>
> > 
> > Per WDDM document, OEM/ODM should keep the ACPI methods (_BQL, _BCM,
> > _BQC) available for compliant to the OS that doesn't support WDDM, e.g.
> > XP, Vista.
> 
> Thanks for the information, so this suggests that acpi interface is
> mostly used for pre-win8 OSes?(does win7 support WDDM?)

Yes, they applied WDDM driver support on Windows 7 platform, but I don't
know how many models only work on WDDM and have broken ACPI
implementation.

> 
> One interesting thing is, if they do not support acpi interface, why
> they even bother to expose this interface in acpi table?
> 
> -Aaron
> 

I don't know, maybe they just thought that doesn't hurt anything because
they only test Windows 7 or Windows 8, dependent on which OS preloaded
and shipped with the machine.

Use OSI string fallback to Windows XP or Vista mode sometimes can
workaround problem when we are lucky. But, bad thing is OEM/ODM also
didn't test XP/Vista code path with other ACPI features, e.g. S3/S4.

Fallback to vendor mode, leave video driver to control backlight is what
we done in platform driver, that's why we have quirk table. That best
situation is we find a general logic to detect does issue machine
shipped with WDDM driver. It's out of my knowledge.


Thanks a lot!
Joey Lee

--
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 Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/22/2013 07:51 PM, Rafael J. Wysocki wrote:
> On Monday, April 22, 2013 05:39:43 PM Aaron Lu wrote:
>> On 04/22/2013 05:42 AM, Rafael J. Wysocki wrote:
>>> On Sunday, April 21, 2013 07:07:04 PM Aaron Lu wrote:
>>>> On 04/21/2013 06:06 AM, Rafael J. Wysocki wrote:
>>>>> On Friday, April 19, 2013 11:15:57 AM Aaron Lu wrote:
>>>>>> On 04/03/2013 03:04 PM, Ben Jencks wrote:
>>>>>>> On 04/02/2013 09:00 AM, Seth Forshee wrote:
>>>>>>>> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
>>>>>>>>>
>>>>>>>>> I really wondered, how Windows handled this, it should have the same
>>>>>>>>> problem, unless they are not using the acpi video interface?
>>>>>>>>
>>>>>>>> I can only guess.
>>>>>>>>
>>>>>>>> I think I remember reading that Windows 8 does smooth backlight
>>>>>>>> transitions, so it may well hit every intermediate brightness value.
>>>>>>>> Lenovo could also be supplying a driver which rounds values to the
>>>>>>>> nearest working value or uses some other interface or something else.
>>>>>>>
>>>>>>> Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
>>>>>>> access to at least 100 distinct brightness levels.
>>>>>>
>>>>>> I just came across a document on win8 backlight control, it has words
>>>>>> like this:
>>>>>> "
>>>>>> In Windows 8, the primary mechanism by which a platform should expose
>>>>>> its display brightness control functionality is the Windows Display
>>>>>> Driver Model (WDDM) miniport Device Driver Interfaces (DDI).
>>>>>> "
>>>>>> So looks like, on win8, ACPI interface is not used for these systems.
>>>>>>
>>>>>> The link for the document is here:
>>>>>> http://msdn.microsoft.com/en-US/library/windows/hardware/jj159305
>>>>>
>>>>> OK, so what does that mean for the issue at hand?
>>>>
>>>> That means, we should not try to use acpi video interface to control
>>>> backlight on these systems if they are in win8 mode.
>>>
>>> In that case, how are we going to indentify "these systems"?
>>
>> Sorry, I don't know.
>>
>> According to the following bug page:
>> https://bugzilla.kernel.org/show_bug.cgi?id=51231
>> "these systems" now include: thinkpad x230, t430s, t530, L430.
> 
> I suppose we need to blacklist them to start with.  I wouldn't like to apply
> any general changes before we know how many different systems are affected by
> this particular issue.

I totally agree.

Thanks,
Aaron

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Mon, Apr 22, 2013 at 08:11:17PM +0800, Aaron Lu wrote:
> On 04/22/2013 07:51 PM, Rafael J. Wysocki wrote:
> > On Monday, April 22, 2013 05:39:43 PM Aaron Lu wrote:
> >> On 04/22/2013 05:42 AM, Rafael J. Wysocki wrote:
> >>> On Sunday, April 21, 2013 07:07:04 PM Aaron Lu wrote:
> >>>> On 04/21/2013 06:06 AM, Rafael J. Wysocki wrote:
> >>>>> On Friday, April 19, 2013 11:15:57 AM Aaron Lu wrote:
> >>>>>> On 04/03/2013 03:04 PM, Ben Jencks wrote:
> >>>>>>> On 04/02/2013 09:00 AM, Seth Forshee wrote:
> >>>>>>>> On Tue, Apr 02, 2013 at 05:08:23PM +0800, Aaron Lu wrote:
> >>>>>>>>>
> >>>>>>>>> I really wondered, how Windows handled this, it should have the same
> >>>>>>>>> problem, unless they are not using the acpi video interface?
> >>>>>>>>
> >>>>>>>> I can only guess.
> >>>>>>>>
> >>>>>>>> I think I remember reading that Windows 8 does smooth backlight
> >>>>>>>> transitions, so it may well hit every intermediate brightness value.
> >>>>>>>> Lenovo could also be supplying a driver which rounds values to the
> >>>>>>>> nearest working value or uses some other interface or something else.
> >>>>>>>
> >>>>>>> Just checked; Windows 8 doesn't use the ACPI interface. It seems to have
> >>>>>>> access to at least 100 distinct brightness levels.
> >>>>>>
> >>>>>> I just came across a document on win8 backlight control, it has words
> >>>>>> like this:
> >>>>>> "
> >>>>>> In Windows 8, the primary mechanism by which a platform should expose
> >>>>>> its display brightness control functionality is the Windows Display
> >>>>>> Driver Model (WDDM) miniport Device Driver Interfaces (DDI).
> >>>>>> "
> >>>>>> So looks like, on win8, ACPI interface is not used for these systems.
> >>>>>>
> >>>>>> The link for the document is here:
> >>>>>> http://msdn.microsoft.com/en-US/library/windows/hardware/jj159305
> >>>>>
> >>>>> OK, so what does that mean for the issue at hand?
> >>>>
> >>>> That means, we should not try to use acpi video interface to control
> >>>> backlight on these systems if they are in win8 mode.
> >>>
> >>> In that case, how are we going to indentify "these systems"?
> >>
> >> Sorry, I don't know.
> >>
> >> According to the following bug page:
> >> https://bugzilla.kernel.org/show_bug.cgi?id=51231
> >> "these systems" now include: thinkpad x230, t430s, t530, L430.

Also W530, X1 Carbon, and Edge E330.

> > I suppose we need to blacklist them to start with.  I wouldn't like to apply
> > any general changes before we know how many different systems are affected by
> > this particular issue.
> 
> I totally agree.

Are you sure that all of these machines have an alternate interface for
userspace to change the brightness? I know that I've got a non-Lenovo
machine with nVidia graphics for which nouveau exposes no backlight
control, and I have no idea what controls are exposed by the proprietary
graphics drivers.

Seth

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/22/2013 09:06 PM, Seth Forshee wrote:
> On Mon, Apr 22, 2013 at 08:11:17PM +0800, Aaron Lu wrote:
>> On 04/22/2013 07:51 PM, Rafael J. Wysocki wrote:
>>> I suppose we need to blacklist them to start with.  I wouldn't like to apply
>>> any general changes before we know how many different systems are affected by
>>> this particular issue.
>>
>> I totally agree.
>
> Are you sure that all of these machines have an alternate interface for
> userspace to change the brightness? I know that I've got a non-Lenovo
> machine with nVidia graphics for which nouveau exposes no backlight

What's the problem with this system then? The same as the thinkpads in
bug 51231?

-Aaron

> control, and I have no idea what controls are exposed by the proprietary
> graphics drivers.
>
> Seth

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Mon, Apr 22, 2013 at 09:40:00PM +0800, Aaron Lu wrote:
> On 04/22/2013 09:06 PM, Seth Forshee wrote:
> >On Mon, Apr 22, 2013 at 08:11:17PM +0800, Aaron Lu wrote:
> >>On 04/22/2013 07:51 PM, Rafael J. Wysocki wrote:
> >>>I suppose we need to blacklist them to start with.  I wouldn't like to apply
> >>>any general changes before we know how many different systems are affected by
> >>>this particular issue.
> >>
> >>I totally agree.
> >
> >Are you sure that all of these machines have an alternate interface for
> >userspace to change the brightness? I know that I've got a non-Lenovo
> >machine with nVidia graphics for which nouveau exposes no backlight
> 
> What's the problem with this system then? The same as the thinkpads in
> bug 51231?

There's no problem with that system; it has another working backlight
interface. I'm only mentioning it to demonstrate that taking away the
ACPI interface could leave some machines without any backlight control
mechanism that Linux currently supports.

Seth

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Aaron Lu]

On 04/22/2013 09:56 PM, Seth Forshee wrote:
> On Mon, Apr 22, 2013 at 09:40:00PM +0800, Aaron Lu wrote:
>> On 04/22/2013 09:06 PM, Seth Forshee wrote:
>>> On Mon, Apr 22, 2013 at 08:11:17PM +0800, Aaron Lu wrote:
>>>> On 04/22/2013 07:51 PM, Rafael J. Wysocki wrote:
>>>>> I suppose we need to blacklist them to start with.  I wouldn't like to apply
>>>>> any general changes before we know how many different systems are affected by
>>>>> this particular issue.
>>>>
>>>> I totally agree.
>>>
>>> Are you sure that all of these machines have an alternate interface for
>>> userspace to change the brightness? I know that I've got a non-Lenovo
>>> machine with nVidia graphics for which nouveau exposes no backlight
>>
>> What's the problem with this system then? The same as the thinkpads in
>> bug 51231?
>
> There's no problem with that system; it has another working backlight
> interface. I'm only mentioning it to demonstrate that taking away the
> ACPI interface could leave some machines without any backlight control
> mechanism that Linux currently supports.

I think we just need take away the ACPI interface for these known broken
systems for now, that would require: check if we are in win8 mode, if
so, disable acpi video interface for these known broken systems.

I think video_detect_dmi_table is the proper place to do this.

-Aaron

Re: [PATCH] ACPI: Disable Windows 8 compatibility for some Lenovo ThinkPads

[Seth Forshee]

On Mon, Apr 22, 2013 at 10:07:32PM +0800, Aaron Lu wrote:
> On 04/22/2013 09:56 PM, Seth Forshee wrote:
> >On Mon, Apr 22, 2013 at 09:40:00PM +0800, Aaron Lu wrote:
> >>On 04/22/2013 09:06 PM, Seth Forshee wrote:
> >>>On Mon, Apr 22, 2013 at 08:11:17PM +0800, Aaron Lu wrote:
> >>>>On 04/22/2013 07:51 PM, Rafael J. Wysocki wrote:
> >>>>>I suppose we need to blacklist them to start with.  I wouldn't like to apply
> >>>>>any general changes before we know how many different systems are affected by
> >>>>>this particular issue.
> >>>>
> >>>>I totally agree.
> >>>
> >>>Are you sure that all of these machines have an alternate interface for
> >>>userspace to change the brightness? I know that I've got a non-Lenovo
> >>>machine with nVidia graphics for which nouveau exposes no backlight
> >>
> >>What's the problem with this system then? The same as the thinkpads in
> >>bug 51231?
> >
> >There's no problem with that system; it has another working backlight
> >interface. I'm only mentioning it to demonstrate that taking away the
> >ACPI interface could leave some machines without any backlight control
> >mechanism that Linux currently supports.
> 
> I think we just need take away the ACPI interface for these known broken
> systems for now, that would require: check if we are in win8 mode, if
> so, disable acpi video interface for these known broken systems.
> 
> I think video_detect_dmi_table is the proper place to do this.

Well, I suppose that's better than what we've got now. I'll post some
patches to the bug for testing.

Seth

Re: [PATCH 5/5] acpi_video: Don't handle ACPI brightness notifications by default

[Aaron Lu]

On 03/08/2013 03:39 AM, Seth Forshee wrote:
> Windows 8 requires all backlight interfaces to report 101 brightness
> values, and as a result we're starting to see machines with that many
> brightness levels in _BCL. For machines which send these notifications
> when the brightness up/down keys are pressed this means a lot of key
> presses to get any kind of noticeable change in brightness.
> 
> For a while now we've had the ability to disable in-kernel handling of
> notifications via the video.brightness_switch_enabled parameter. Change
> this to default to off, and let userspace choose more reasonable
> increments for changing the brightness.

I just found one more reason for this param to default 0.

We are going to separate the backlight interface control and event
notification functionalities of the ACPI video module, it is highly
possible a lot of systems will use a combination of the event
notification handler and intel_backlight interface. So it doesn't make
sense to let video module to do any adjustment on its own if user space
has chosen a different interface to use. Actually, it can cause problems
as in ASUS's case:
https://bugzilla.kernel.org/show_bug.cgi?id=52951

The problem there is, on hotkey brightness up, the video module will
adjust the brightness level first and since its _BQC is broken, it gets
a wrong number(too low or too high or whatever) and then does the _BCM
call. The _BCM method works. Then user space picks the intel_backlight
to do the adjustment, but since the _BCM already sets a wrong value, the
user space's adjustment is affected too. The result is, user has only
two visible levels, very low and very high.

This only occurs on -rc3, since we removed the
acpi_video_verify_backlight_support from acpi_video_switch_brightness
function. So either we make this param default to 0, or we make a new
function to avoid brightness switch in video module for Win8 systems.

I prefer to set this param to 0 by default. What do you guys think?

Thanks,
Aaron

> 
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  drivers/acpi/video.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 6a19bf7..431b22e 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -69,7 +69,7 @@ MODULE_AUTHOR("Bruno Ducrot");
>  MODULE_DESCRIPTION("ACPI Video Driver");
>  MODULE_LICENSE("GPL");
>  
> -static bool brightness_switch_enabled = 1;
> +static bool brightness_switch_enabled = 0;
>  module_param(brightness_switch_enabled, bool, 0644);
>  
>  /*
> 

Re: [PATCH 5/5] acpi_video: Don't handle ACPI brightness notifications by default

[Rafael J. Wysocki]

On Friday, August 02, 2013 01:55:47 PM Aaron Lu wrote:
> On 03/08/2013 03:39 AM, Seth Forshee wrote:
> > Windows 8 requires all backlight interfaces to report 101 brightness
> > values, and as a result we're starting to see machines with that many
> > brightness levels in _BCL. For machines which send these notifications
> > when the brightness up/down keys are pressed this means a lot of key
> > presses to get any kind of noticeable change in brightness.
> > 
> > For a while now we've had the ability to disable in-kernel handling of
> > notifications via the video.brightness_switch_enabled parameter. Change
> > this to default to off, and let userspace choose more reasonable
> > increments for changing the brightness.
> 
> I just found one more reason for this param to default 0.

Do you mean video.brightness_switch_enabled?

> We are going to separate the backlight interface control and event
> notification functionalities of the ACPI video module, it is highly
> possible a lot of systems will use a combination of the event
> notification handler and intel_backlight interface. So it doesn't make
> sense to let video module to do any adjustment on its own if user space
> has chosen a different interface to use. Actually, it can cause problems
> as in ASUS's case:
> https://bugzilla.kernel.org/show_bug.cgi?id=52951
> 
> The problem there is, on hotkey brightness up, the video module will
> adjust the brightness level first and since its _BQC is broken, it gets
> a wrong number(too low or too high or whatever) and then does the _BCM
> call. The _BCM method works. Then user space picks the intel_backlight
> to do the adjustment, but since the _BCM already sets a wrong value, the
> user space's adjustment is affected too. The result is, user has only
> two visible levels, very low and very high.
> 
> This only occurs on -rc3, since we removed the
> acpi_video_verify_backlight_support from acpi_video_switch_brightness
> function.

What did we do before -rc2?  Did we address that in any way?

> So either we make this param default to 0, or we make a new
> function to avoid brightness switch in video module for Win8 systems.
> 
> I prefer to set this param to 0 by default. What do you guys think?

That depends on what we did earlier.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 5/5] acpi_video: Don't handle ACPI brightness notifications by default

[Aaron Lu]

On 08/02/2013 10:41 PM, Rafael J. Wysocki wrote:
> On Friday, August 02, 2013 01:55:47 PM Aaron Lu wrote:
>> On 03/08/2013 03:39 AM, Seth Forshee wrote:
>>> Windows 8 requires all backlight interfaces to report 101 brightness
>>> values, and as a result we're starting to see machines with that many
>>> brightness levels in _BCL. For machines which send these notifications
>>> when the brightness up/down keys are pressed this means a lot of key
>>> presses to get any kind of noticeable change in brightness.
>>>
>>> For a while now we've had the ability to disable in-kernel handling of
>>> notifications via the video.brightness_switch_enabled parameter. Change
>>> this to default to off, and let userspace choose more reasonable
>>> increments for changing the brightness.
>>
>> I just found one more reason for this param to default 0.
> 
> Do you mean video.brightness_switch_enabled?

Yes.

> 
>> We are going to separate the backlight interface control and event
>> notification functionalities of the ACPI video module, it is highly
>> possible a lot of systems will use a combination of the event
>> notification handler and intel_backlight interface. So it doesn't make
>> sense to let video module to do any adjustment on its own if user space
>> has chosen a different interface to use. Actually, it can cause problems
>> as in ASUS's case:
>> https://bugzilla.kernel.org/show_bug.cgi?id=52951
>>
>> The problem there is, on hotkey brightness up, the video module will
>> adjust the brightness level first and since its _BQC is broken, it gets
>> a wrong number(too low or too high or whatever) and then does the _BCM
>> call. The _BCM method works. Then user space picks the intel_backlight
>> to do the adjustment, but since the _BCM already sets a wrong value, the
>> user space's adjustment is affected too. The result is, user has only
>> two visible levels, very low and very high.
>>
>> This only occurs on -rc3, since we removed the
>> acpi_video_verify_backlight_support from acpi_video_switch_brightness
>> function.
> 
> What did we do before -rc2?  Did we address that in any way?

No, before rc2, backlight is broken on that system.

In rc2, we added the win8 patch and a fix patch for the hotkey, then
the ACPI video module's backlight control and in kernel brightness
handling is disabled. With the working hotkey and intel_backlight, rc2
works for the system. Then with the revert in rc3, user needs to choose
intel_backlight in xorg.conf but the in kernel brightness handling from
ACPI video module is back. Since video module is broken, it breaks the
backlight hotkey functionality.

Thanks,
Aaron

> 
>> So either we make this param default to 0, or we make a new
>> function to avoid brightness switch in video module for Win8 systems.
>>
>> I prefer to set this param to 0 by default. What do you guys think?
> 
> That depends on what we did earlier.
> 
> Thanks,
> Rafael
> 
> 

Re: [PATCH 5/5] acpi_video: Don't handle ACPI brightness notifications by default

[Rafael J. Wysocki]

On Friday, August 02, 2013 10:52:09 PM Aaron Lu wrote:
> On 08/02/2013 10:41 PM, Rafael J. Wysocki wrote:
> > On Friday, August 02, 2013 01:55:47 PM Aaron Lu wrote:
> >> On 03/08/2013 03:39 AM, Seth Forshee wrote:
> >>> Windows 8 requires all backlight interfaces to report 101 brightness
> >>> values, and as a result we're starting to see machines with that many
> >>> brightness levels in _BCL. For machines which send these notifications
> >>> when the brightness up/down keys are pressed this means a lot of key
> >>> presses to get any kind of noticeable change in brightness.
> >>>
> >>> For a while now we've had the ability to disable in-kernel handling of
> >>> notifications via the video.brightness_switch_enabled parameter. Change
> >>> this to default to off, and let userspace choose more reasonable
> >>> increments for changing the brightness.
> >>
> >> I just found one more reason for this param to default 0.
> > 
> > Do you mean video.brightness_switch_enabled?
> 
> Yes.
> 
> > 
> >> We are going to separate the backlight interface control and event
> >> notification functionalities of the ACPI video module, it is highly
> >> possible a lot of systems will use a combination of the event
> >> notification handler and intel_backlight interface. So it doesn't make
> >> sense to let video module to do any adjustment on its own if user space
> >> has chosen a different interface to use. Actually, it can cause problems
> >> as in ASUS's case:
> >> https://bugzilla.kernel.org/show_bug.cgi?id=52951
> >>
> >> The problem there is, on hotkey brightness up, the video module will
> >> adjust the brightness level first and since its _BQC is broken, it gets
> >> a wrong number(too low or too high or whatever) and then does the _BCM
> >> call. The _BCM method works. Then user space picks the intel_backlight
> >> to do the adjustment, but since the _BCM already sets a wrong value, the
> >> user space's adjustment is affected too. The result is, user has only
> >> two visible levels, very low and very high.
> >>
> >> This only occurs on -rc3, since we removed the
> >> acpi_video_verify_backlight_support from acpi_video_switch_brightness
> >> function.
> > 
> > What did we do before -rc2?  Did we address that in any way?
> 
> No, before rc2, backlight is broken on that system.

Well, it will have to stay that way in 3.11 I'm afraid, unless we have a fix
or a workaround that is *guaranteed* not to introduce any new issues on any
systems.

> In rc2, we added the win8 patch and a fix patch for the hotkey, then
> the ACPI video module's backlight control and in kernel brightness
> handling is disabled. With the working hotkey and intel_backlight, rc2
> works for the system. Then with the revert in rc3, user needs to choose
> intel_backlight in xorg.conf but the in kernel brightness handling from
> ACPI video module is back. Since video module is broken, it breaks the
> backlight hotkey functionality.

I guess we need to revert the hotkey fix too (that is efaa14c, right?)
then, is that correct?  And try to do something smarter for 3.12?

Rafael

Re: [PATCH 5/5] acpi_video: Don't handle ACPI brightness notifications by default

[Aaron Lu]

On 08/03/2013 08:26 AM, Rafael J. Wysocki wrote:
> On Friday, August 02, 2013 10:52:09 PM Aaron Lu wrote:
>> On 08/02/2013 10:41 PM, Rafael J. Wysocki wrote:
>>> On Friday, August 02, 2013 01:55:47 PM Aaron Lu wrote:
>>>> On 03/08/2013 03:39 AM, Seth Forshee wrote:
>>>>> Windows 8 requires all backlight interfaces to report 101 brightness
>>>>> values, and as a result we're starting to see machines with that many
>>>>> brightness levels in _BCL. For machines which send these notifications
>>>>> when the brightness up/down keys are pressed this means a lot of key
>>>>> presses to get any kind of noticeable change in brightness.
>>>>>
>>>>> For a while now we've had the ability to disable in-kernel handling of
>>>>> notifications via the video.brightness_switch_enabled parameter. Change
>>>>> this to default to off, and let userspace choose more reasonable
>>>>> increments for changing the brightness.
>>>>
>>>> I just found one more reason for this param to default 0.
>>>
>>> Do you mean video.brightness_switch_enabled?
>>
>> Yes.
>>
>>>
>>>> We are going to separate the backlight interface control and event
>>>> notification functionalities of the ACPI video module, it is highly
>>>> possible a lot of systems will use a combination of the event
>>>> notification handler and intel_backlight interface. So it doesn't make
>>>> sense to let video module to do any adjustment on its own if user space
>>>> has chosen a different interface to use. Actually, it can cause problems
>>>> as in ASUS's case:
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=52951
>>>>
>>>> The problem there is, on hotkey brightness up, the video module will
>>>> adjust the brightness level first and since its _BQC is broken, it gets
>>>> a wrong number(too low or too high or whatever) and then does the _BCM
>>>> call. The _BCM method works. Then user space picks the intel_backlight
>>>> to do the adjustment, but since the _BCM already sets a wrong value, the
>>>> user space's adjustment is affected too. The result is, user has only
>>>> two visible levels, very low and very high.
>>>>
>>>> This only occurs on -rc3, since we removed the
>>>> acpi_video_verify_backlight_support from acpi_video_switch_brightness
>>>> function.
>>>
>>> What did we do before -rc2?  Did we address that in any way?
>>
>> No, before rc2, backlight is broken on that system.
> 
> Well, it will have to stay that way in 3.11 I'm afraid, unless we have a fix
> or a workaround that is *guaranteed* not to introduce any new issues on any
> systems.
> 
>> In rc2, we added the win8 patch and a fix patch for the hotkey, then
>> the ACPI video module's backlight control and in kernel brightness
>> handling is disabled. With the working hotkey and intel_backlight, rc2
>> works for the system. Then with the revert in rc3, user needs to choose
>> intel_backlight in xorg.conf but the in kernel brightness handling from
>> ACPI video module is back. Since video module is broken, it breaks the
>> backlight hotkey functionality.
> 
> I guess we need to revert the hotkey fix too (that is efaa14c, right?)
> then, is that correct?  And try to do something smarter for 3.12?

With or without commit efaa14c, hotkey for backlight is broken out of
box for the affected systems(ASUS N56VZ and N56VJ). But with that commit,
user has a chance of getting a working backlight with hotkey by specifying
intel_backlight and adding the video.brightness_switch_enabled=0. So I
think we can keep that commit.

Thanks,
Aaron

Re: [PATCH 5/5] acpi_video: Don't handle ACPI brightness notifications by default

[Rafael J. Wysocki]

On Saturday, August 03, 2013 05:46:02 PM Aaron Lu wrote:
> On 08/03/2013 08:26 AM, Rafael J. Wysocki wrote:
> > On Friday, August 02, 2013 10:52:09 PM Aaron Lu wrote:
> >> On 08/02/2013 10:41 PM, Rafael J. Wysocki wrote:
> >>> On Friday, August 02, 2013 01:55:47 PM Aaron Lu wrote:
> >>>> On 03/08/2013 03:39 AM, Seth Forshee wrote:
> >>>>> Windows 8 requires all backlight interfaces to report 101 brightness
> >>>>> values, and as a result we're starting to see machines with that many
> >>>>> brightness levels in _BCL. For machines which send these notifications
> >>>>> when the brightness up/down keys are pressed this means a lot of key
> >>>>> presses to get any kind of noticeable change in brightness.
> >>>>>
> >>>>> For a while now we've had the ability to disable in-kernel handling of
> >>>>> notifications via the video.brightness_switch_enabled parameter. Change
> >>>>> this to default to off, and let userspace choose more reasonable
> >>>>> increments for changing the brightness.
> >>>>
> >>>> I just found one more reason for this param to default 0.
> >>>
> >>> Do you mean video.brightness_switch_enabled?
> >>
> >> Yes.
> >>
> >>>
> >>>> We are going to separate the backlight interface control and event
> >>>> notification functionalities of the ACPI video module, it is highly
> >>>> possible a lot of systems will use a combination of the event
> >>>> notification handler and intel_backlight interface. So it doesn't make
> >>>> sense to let video module to do any adjustment on its own if user space
> >>>> has chosen a different interface to use. Actually, it can cause problems
> >>>> as in ASUS's case:
> >>>> https://bugzilla.kernel.org/show_bug.cgi?id=52951
> >>>>
> >>>> The problem there is, on hotkey brightness up, the video module will
> >>>> adjust the brightness level first and since its _BQC is broken, it gets
> >>>> a wrong number(too low or too high or whatever) and then does the _BCM
> >>>> call. The _BCM method works. Then user space picks the intel_backlight
> >>>> to do the adjustment, but since the _BCM already sets a wrong value, the
> >>>> user space's adjustment is affected too. The result is, user has only
> >>>> two visible levels, very low and very high.
> >>>>
> >>>> This only occurs on -rc3, since we removed the
> >>>> acpi_video_verify_backlight_support from acpi_video_switch_brightness
> >>>> function.
> >>>
> >>> What did we do before -rc2?  Did we address that in any way?
> >>
> >> No, before rc2, backlight is broken on that system.
> > 
> > Well, it will have to stay that way in 3.11 I'm afraid, unless we have a fix
> > or a workaround that is *guaranteed* not to introduce any new issues on any
> > systems.
> > 
> >> In rc2, we added the win8 patch and a fix patch for the hotkey, then
> >> the ACPI video module's backlight control and in kernel brightness
> >> handling is disabled. With the working hotkey and intel_backlight, rc2
> >> works for the system. Then with the revert in rc3, user needs to choose
> >> intel_backlight in xorg.conf but the in kernel brightness handling from
> >> ACPI video module is back. Since video module is broken, it breaks the
> >> backlight hotkey functionality.
> > 
> > I guess we need to revert the hotkey fix too (that is efaa14c, right?)
> > then, is that correct?  And try to do something smarter for 3.12?
> 
> With or without commit efaa14c, hotkey for backlight is broken out of
> box for the affected systems(ASUS N56VZ and N56VJ). But with that commit,
> user has a chance of getting a working backlight with hotkey by specifying
> intel_backlight and adding the video.brightness_switch_enabled=0. So I
> think we can keep that commit.

What about the "boot to black screen" problem, then?

Rafael

Re: [PATCH 5/5] acpi_video: Don't handle ACPI brightness notifications by default

[Aaron Lu]

On 08/03/2013 07:23 PM, Rafael J. Wysocki wrote:
> On Saturday, August 03, 2013 05:46:02 PM Aaron Lu wrote:
>> On 08/03/2013 08:26 AM, Rafael J. Wysocki wrote:
>>> On Friday, August 02, 2013 10:52:09 PM Aaron Lu wrote:
>>>> On 08/02/2013 10:41 PM, Rafael J. Wysocki wrote:
>>>>> On Friday, August 02, 2013 01:55:47 PM Aaron Lu wrote:
>>>>>> On 03/08/2013 03:39 AM, Seth Forshee wrote:
>>>>>>> Windows 8 requires all backlight interfaces to report 101 brightness
>>>>>>> values, and as a result we're starting to see machines with that many
>>>>>>> brightness levels in _BCL. For machines which send these notifications
>>>>>>> when the brightness up/down keys are pressed this means a lot of key
>>>>>>> presses to get any kind of noticeable change in brightness.
>>>>>>>
>>>>>>> For a while now we've had the ability to disable in-kernel handling of
>>>>>>> notifications via the video.brightness_switch_enabled parameter. Change
>>>>>>> this to default to off, and let userspace choose more reasonable
>>>>>>> increments for changing the brightness.
>>>>>>
>>>>>> I just found one more reason for this param to default 0.
>>>>>
>>>>> Do you mean video.brightness_switch_enabled?
>>>>
>>>> Yes.
>>>>
>>>>>
>>>>>> We are going to separate the backlight interface control and event
>>>>>> notification functionalities of the ACPI video module, it is highly
>>>>>> possible a lot of systems will use a combination of the event
>>>>>> notification handler and intel_backlight interface. So it doesn't make
>>>>>> sense to let video module to do any adjustment on its own if user space
>>>>>> has chosen a different interface to use. Actually, it can cause problems
>>>>>> as in ASUS's case:
>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=52951
>>>>>>
>>>>>> The problem there is, on hotkey brightness up, the video module will
>>>>>> adjust the brightness level first and since its _BQC is broken, it gets
>>>>>> a wrong number(too low or too high or whatever) and then does the _BCM
>>>>>> call. The _BCM method works. Then user space picks the intel_backlight
>>>>>> to do the adjustment, but since the _BCM already sets a wrong value, the
>>>>>> user space's adjustment is affected too. The result is, user has only
>>>>>> two visible levels, very low and very high.
>>>>>>
>>>>>> This only occurs on -rc3, since we removed the
>>>>>> acpi_video_verify_backlight_support from acpi_video_switch_brightness
>>>>>> function.
>>>>>
>>>>> What did we do before -rc2?  Did we address that in any way?
>>>>
>>>> No, before rc2, backlight is broken on that system.
>>>
>>> Well, it will have to stay that way in 3.11 I'm afraid, unless we have a fix
>>> or a workaround that is *guaranteed* not to introduce any new issues on any
>>> systems.
>>>
>>>> In rc2, we added the win8 patch and a fix patch for the hotkey, then
>>>> the ACPI video module's backlight control and in kernel brightness
>>>> handling is disabled. With the working hotkey and intel_backlight, rc2
>>>> works for the system. Then with the revert in rc3, user needs to choose
>>>> intel_backlight in xorg.conf but the in kernel brightness handling from
>>>> ACPI video module is back. Since video module is broken, it breaks the
>>>> backlight hotkey functionality.
>>>
>>> I guess we need to revert the hotkey fix too (that is efaa14c, right?)
>>> then, is that correct?  And try to do something smarter for 3.12?
>>
>> With or without commit efaa14c, hotkey for backlight is broken out of
>> box for the affected systems(ASUS N56VZ and N56VJ). But with that commit,
>> user has a chance of getting a working backlight with hotkey by specifying
>> intel_backlight and adding the video.brightness_switch_enabled=0. So I
>> think we can keep that commit.
> 
> What about the "boot to black screen" problem, then?

You mean this one?
https://lkml.org/lkml/2013/7/30/814

-Aaron

Re: [PATCH 5/5] acpi_video: Don't handle ACPI brightness notifications by default

[Rafael J. Wysocki]

On Saturday, August 03, 2013 08:10:12 PM Aaron Lu wrote:
> On 08/03/2013 07:23 PM, Rafael J. Wysocki wrote:
> > On Saturday, August 03, 2013 05:46:02 PM Aaron Lu wrote:
> >> On 08/03/2013 08:26 AM, Rafael J. Wysocki wrote:
> >>> On Friday, August 02, 2013 10:52:09 PM Aaron Lu wrote:
> >>>> On 08/02/2013 10:41 PM, Rafael J. Wysocki wrote:
> >>>>> On Friday, August 02, 2013 01:55:47 PM Aaron Lu wrote:
> >>>>>> On 03/08/2013 03:39 AM, Seth Forshee wrote:
> >>>>>>> Windows 8 requires all backlight interfaces to report 101 brightness
> >>>>>>> values, and as a result we're starting to see machines with that many
> >>>>>>> brightness levels in _BCL. For machines which send these notifications
> >>>>>>> when the brightness up/down keys are pressed this means a lot of key
> >>>>>>> presses to get any kind of noticeable change in brightness.
> >>>>>>>
> >>>>>>> For a while now we've had the ability to disable in-kernel handling of
> >>>>>>> notifications via the video.brightness_switch_enabled parameter. Change
> >>>>>>> this to default to off, and let userspace choose more reasonable
> >>>>>>> increments for changing the brightness.
> >>>>>>
> >>>>>> I just found one more reason for this param to default 0.
> >>>>>
> >>>>> Do you mean video.brightness_switch_enabled?
> >>>>
> >>>> Yes.
> >>>>
> >>>>>
> >>>>>> We are going to separate the backlight interface control and event
> >>>>>> notification functionalities of the ACPI video module, it is highly
> >>>>>> possible a lot of systems will use a combination of the event
> >>>>>> notification handler and intel_backlight interface. So it doesn't make
> >>>>>> sense to let video module to do any adjustment on its own if user space
> >>>>>> has chosen a different interface to use. Actually, it can cause problems
> >>>>>> as in ASUS's case:
> >>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=52951
> >>>>>>
> >>>>>> The problem there is, on hotkey brightness up, the video module will
> >>>>>> adjust the brightness level first and since its _BQC is broken, it gets
> >>>>>> a wrong number(too low or too high or whatever) and then does the _BCM
> >>>>>> call. The _BCM method works. Then user space picks the intel_backlight
> >>>>>> to do the adjustment, but since the _BCM already sets a wrong value, the
> >>>>>> user space's adjustment is affected too. The result is, user has only
> >>>>>> two visible levels, very low and very high.
> >>>>>>
> >>>>>> This only occurs on -rc3, since we removed the
> >>>>>> acpi_video_verify_backlight_support from acpi_video_switch_brightness
> >>>>>> function.
> >>>>>
> >>>>> What did we do before -rc2?  Did we address that in any way?
> >>>>
> >>>> No, before rc2, backlight is broken on that system.
> >>>
> >>> Well, it will have to stay that way in 3.11 I'm afraid, unless we have a fix
> >>> or a workaround that is *guaranteed* not to introduce any new issues on any
> >>> systems.
> >>>
> >>>> In rc2, we added the win8 patch and a fix patch for the hotkey, then
> >>>> the ACPI video module's backlight control and in kernel brightness
> >>>> handling is disabled. With the working hotkey and intel_backlight, rc2
> >>>> works for the system. Then with the revert in rc3, user needs to choose
> >>>> intel_backlight in xorg.conf but the in kernel brightness handling from
> >>>> ACPI video module is back. Since video module is broken, it breaks the
> >>>> backlight hotkey functionality.
> >>>
> >>> I guess we need to revert the hotkey fix too (that is efaa14c, right?)
> >>> then, is that correct?  And try to do something smarter for 3.12?
> >>
> >> With or without commit efaa14c, hotkey for backlight is broken out of
> >> box for the affected systems(ASUS N56VZ and N56VJ). But with that commit,
> >> user has a chance of getting a working backlight with hotkey by specifying
> >> intel_backlight and adding the video.brightness_switch_enabled=0. So I
> >> think we can keep that commit.
> > 
> > What about the "boot to black screen" problem, then?
> 
> You mean this one?
> https://lkml.org/lkml/2013/7/30/814

Yes. :-)

Re: [PATCH 5/5] acpi_video: Don't handle ACPI brightness notifications by default

[Aaron Lu]

On 08/04/2013 06:07 AM, Rafael J. Wysocki wrote:
> On Saturday, August 03, 2013 08:10:12 PM Aaron Lu wrote:
>> On 08/03/2013 07:23 PM, Rafael J. Wysocki wrote:
>>> On Saturday, August 03, 2013 05:46:02 PM Aaron Lu wrote:
>>>> On 08/03/2013 08:26 AM, Rafael J. Wysocki wrote:
>>>>> On Friday, August 02, 2013 10:52:09 PM Aaron Lu wrote:
>>>>>> On 08/02/2013 10:41 PM, Rafael J. Wysocki wrote:
>>>>>>> On Friday, August 02, 2013 01:55:47 PM Aaron Lu wrote:
>>>>>>>> On 03/08/2013 03:39 AM, Seth Forshee wrote:
>>>>>>>>> Windows 8 requires all backlight interfaces to report 101 brightness
>>>>>>>>> values, and as a result we're starting to see machines with that many
>>>>>>>>> brightness levels in _BCL. For machines which send these notifications
>>>>>>>>> when the brightness up/down keys are pressed this means a lot of key
>>>>>>>>> presses to get any kind of noticeable change in brightness.
>>>>>>>>>
>>>>>>>>> For a while now we've had the ability to disable in-kernel handling of
>>>>>>>>> notifications via the video.brightness_switch_enabled parameter. Change
>>>>>>>>> this to default to off, and let userspace choose more reasonable
>>>>>>>>> increments for changing the brightness.
>>>>>>>>
>>>>>>>> I just found one more reason for this param to default 0.
>>>>>>>
>>>>>>> Do you mean video.brightness_switch_enabled?
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>>
>>>>>>>> We are going to separate the backlight interface control and event
>>>>>>>> notification functionalities of the ACPI video module, it is highly
>>>>>>>> possible a lot of systems will use a combination of the event
>>>>>>>> notification handler and intel_backlight interface. So it doesn't make
>>>>>>>> sense to let video module to do any adjustment on its own if user space
>>>>>>>> has chosen a different interface to use. Actually, it can cause problems
>>>>>>>> as in ASUS's case:
>>>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=52951
>>>>>>>>
>>>>>>>> The problem there is, on hotkey brightness up, the video module will
>>>>>>>> adjust the brightness level first and since its _BQC is broken, it gets
>>>>>>>> a wrong number(too low or too high or whatever) and then does the _BCM
>>>>>>>> call. The _BCM method works. Then user space picks the intel_backlight
>>>>>>>> to do the adjustment, but since the _BCM already sets a wrong value, the
>>>>>>>> user space's adjustment is affected too. The result is, user has only
>>>>>>>> two visible levels, very low and very high.
>>>>>>>>
>>>>>>>> This only occurs on -rc3, since we removed the
>>>>>>>> acpi_video_verify_backlight_support from acpi_video_switch_brightness
>>>>>>>> function.
>>>>>>>
>>>>>>> What did we do before -rc2?  Did we address that in any way?
>>>>>>
>>>>>> No, before rc2, backlight is broken on that system.
>>>>>
>>>>> Well, it will have to stay that way in 3.11 I'm afraid, unless we have a fix
>>>>> or a workaround that is *guaranteed* not to introduce any new issues on any
>>>>> systems.
>>>>>
>>>>>> In rc2, we added the win8 patch and a fix patch for the hotkey, then
>>>>>> the ACPI video module's backlight control and in kernel brightness
>>>>>> handling is disabled. With the working hotkey and intel_backlight, rc2
>>>>>> works for the system. Then with the revert in rc3, user needs to choose
>>>>>> intel_backlight in xorg.conf but the in kernel brightness handling from
>>>>>> ACPI video module is back. Since video module is broken, it breaks the
>>>>>> backlight hotkey functionality.
>>>>>
>>>>> I guess we need to revert the hotkey fix too (that is efaa14c, right?)
>>>>> then, is that correct?  And try to do something smarter for 3.12?
>>>>
>>>> With or without commit efaa14c, hotkey for backlight is broken out of
>>>> box for the affected systems(ASUS N56VZ and N56VJ). But with that commit,
>>>> user has a chance of getting a working backlight with hotkey by specifying
>>>> intel_backlight and adding the video.brightness_switch_enabled=0. So I
>>>> think we can keep that commit.
>>>
>>> What about the "boot to black screen" problem, then?
>>
>> You mean this one?
>> https://lkml.org/lkml/2013/7/30/814
> 
> Yes. :-)

That is due to the broken _BQC provided by firmware, not that commit.
After we enhance the quirk for _BQC, the problem will be gone.

-Aaron