[PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Darren Salt]

This maps the brightness control events to one of two keys, either
KEY_BRIGHTNESSDOWN or KEY_BRIGHTNESSUP, as needed.

Some mapping has to be done due to the fact that the BIOS reports them as
<base value> + <current brightness index>; the selection is done according to
the sign of the change in brightness (if this is 0, no keypress is reported).

(Ref. http://lists.alioth.debian.org/pipermail/debian-eeepc-devel/2009-April/002001.html)

Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>

diff -u a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
--- a/drivers/platform/x86/eeepc-laptop.c	2009-03-24 17:32:56.000000000 +0000
+++ b/drivers/platform/x86/eeepc-laptop.c	2009-04-03 13:24:59.000000000 +0100
@@ -166,6 +166,8 @@
 	{KE_KEY, 0x1b, KEY_ZOOM },
 	{KE_KEY, 0x1c, KEY_PROG2 },
 	{KE_KEY, 0x1d, KEY_PROG3 },
+	{KE_KEY, NOTIFY_BRN_MIN,     KEY_BRIGHTNESSDOWN },
+	{KE_KEY, NOTIFY_BRN_MIN + 2, KEY_BRIGHTNESSUP },
 	{KE_KEY, 0x30, KEY_SWITCHVIDEOMODE },
 	{KE_KEY, 0x31, KEY_SWITCHVIDEOMODE },
 	{KE_KEY, 0x32, KEY_SWITCHVIDEOMODE },
@@ -512,11 +514,17 @@
 	return 0;
 }
 
-static void notify_brn(void)
+static int notify_brn(void)
 {
+	/* returns the *previous* brightness, or -1 */
 	struct backlight_device *bd = eeepc_backlight_device;
 	if (bd)
+	{
+		int old = bd->props.brightness;
 		bd->props.brightness = read_brightness(bd);
+		return old;
+	}
+	return -1;
 }
 
 static void eeepc_rfkill_notify(acpi_handle handle, u32 event, void *data)
@@ -558,17 +566,34 @@
 {
 	static struct key_entry *key;
 	u16 count;
+	int brn = -2;
 
 	if (!ehotk)
 		return;
 	if (event >= NOTIFY_BRN_MIN && event <= NOTIFY_BRN_MAX)
-		notify_brn();
+		brn = notify_brn();
 	count = ehotk->event_count[event % 128]++;
 	acpi_bus_generate_proc_event(ehotk->device, event, count);
 	acpi_bus_generate_netlink_event(ehotk->device->pnp.device_class,
 					dev_name(&ehotk->device->dev), event,
 					count);
 	if (ehotk->inputdev) {
+		if (brn != -2)
+		{
+			/* brightness-change events need special
+			 * handling for conversion to key events
+			 */
+			if (brn == -1)
+				brn = event;
+			else
+				brn += NOTIFY_BRN_MIN;
+			if (event < brn)
+				event = NOTIFY_BRN_MIN; /* brightness down */
+			else if (event > brn)
+				event = NOTIFY_BRN_MIN + 2; /* ... up */
+			else
+				event = NOTIFY_BRN_MIN + 1; /* ... unchanged */
+		}
 		key = eepc_get_entry_by_scancode(event);
 		if (key) {
 			switch (key->type) {


-- 
| Darren Salt    | linux or ds at              | nr. Ashington, | Toon
| RISC OS, Linux | youmustbejoking,demon,co,uk | Northumberland | Army
| + Output *more* particulate pollutants.      BUFFER AGAINST GLOBAL WARMING.

I cut down trees, I eat my lunch, I go to the lavatory.

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Matthew Garrett]

On Fri, Apr 03, 2009 at 06:57:50PM +0100, Darren Salt wrote:
> This maps the brightness control events to one of two keys, either
> KEY_BRIGHTNESSDOWN or KEY_BRIGHTNESSUP, as needed.
> 
> Some mapping has to be done due to the fact that the BIOS reports them as
> <base value> + <current brightness index>; the selection is done according to
> the sign of the change in brightness (if this is 0, no keypress is reported).
> 
> (Ref. http://lists.alioth.debian.org/pipermail/debian-eeepc-devel/2009-April/002001.html)
> 
> Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>

The reason I didn't do this is that the Eee changes the input brightness 
in hardware, which means reporting it via the input layer as well can 
cause a single keypress to raise the brightness by two steps - one in 
hardware and one triggered by userland's response to the key press. I'd 
be a little bit wary of this causing problems.

On the other hand, the default behaviour of the acpi video driver is to 
change the brightness itself and then also to send the even to 
userspace, so I guess if it was going to break things it probably would 
have done already...

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

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Corentin Chary]

> On the other hand, the default behaviour of the acpi video driver is to
> change the brightness itself and then also to send the even to
> userspace, so I guess if it was going to break things it probably would
> have done already...

So, I think this patch is ok.

But there is a thing I don't like is
int brn = -2;
brn = notify_brn();
if (brn != -2)

How can brn be -2 ? And why -2 ?

-- 
Corentin Chary
http://xf.iksaif.net

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Darren Salt]

I demand that Corentin Chary may or may not have written...

>> On the other hand, the default behaviour of the acpi video driver is to
>> change the brightness itself and then also to send the even to
>> userspace, so I guess if it was going to break things it probably would
>> have done already...

> So, I think this patch is ok.

> But there is a thing I don't like is
> int brn = -2;
> brn = notify_brn();
> if (brn != -2)

> How can brn be -2?

If notify_brn() wasn't called, it will be.

> And why -2?

Because notify_brn() won't return it (and if it ever does, it needs to be
fixed). (Yes, I know, "magic numbers" and all that...)

-- 
| Darren Salt    | linux or ds at              | nr. Ashington, | Toon
| RISC OS, Linux | youmustbejoking,demon,co,uk | Northumberland | Army
| + Lobby friends, family, business, government.    WE'RE KILLING THE PLANET.

Opinions for rent. Reasonable rates.

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Corentin Chary]

>> How can brn be -2?
> If notify_brn() wasn't called, it will be.
Oh, yeah, I miss the if() before notify_brn()

>> And why -2?
> Because notify_brn() won't return it (and if it ever does, it needs to be
> fixed). (Yes, I know, "magic numbers" and all that...)
Maybe a negative known error code could be used here


-- 
Corentin Chary
http://xf.iksaif.net

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Darren Salt]

I demand that Corentin Chary may or may not have written...

>>> How can brn be -2?
>> If notify_brn() wasn't called, it will be.

> Oh, yeah, I miss the if() before notify_brn()

Easily missed. ;-)

>>> And why -2?
>> Because notify_brn() won't return it (and if it ever does, it needs to be
>> fixed). (Yes, I know, "magic numbers" and all that...)

> Maybe a negative known error code could be used here

I see the following:

* read_acpi_int() returns 0 and writes the brightness setting to *val, or
  returns -1 and writes -1 to *val if the ACPI call failed.

* get_acpi() returns whatever was written to value by read_acpi_int(), or
  -ENODEV if there's no ACPI method which can be called.

* read_brightness is a trivial wrapper for get_acpi().

* notify_brn() stores the result of read_brightness and returns the
  previously-stored value, so it can store then later return -ENODEV, -1 or
  the brightness setting.

This makes -ENODEV a suitable value. Replacing that -1 with something other
than -ENODEV might be good, but I don't think that that really matters right
now (though I've replaced the "!= -1" in the original version of the patch
with "< 0").

Revised patch follows...

---
eeepc-laptop: report brightness control events via the input layer

This maps the brightness control events to one of two keys, either
KEY_BRIGHTNESSDOWN or KEY_BRIGHTNESSUP, as needed.

Some mapping has to be done due to the fact that the BIOS reports them as
<base value> + <current brightness index>; the selection is done according to
the sign of the change in brightness (if this is 0, no keypress is reported).

(Ref. http://lists.alioth.debian.org/pipermail/debian-eeepc-devel/2009-April/002001.html)

Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>

--- a/drivers/platform/x86/eeepc-laptop.c	2009-03-24 17:32:56.000000000 +0000
+++ b/drivers/platform/x86/eeepc-laptop.c	2009-04-03 23:37:34.000000000 +0100
@@ -166,6 +166,8 @@
 	{KE_KEY, 0x1b, KEY_ZOOM },
 	{KE_KEY, 0x1c, KEY_PROG2 },
 	{KE_KEY, 0x1d, KEY_PROG3 },
+	{KE_KEY, NOTIFY_BRN_MIN,     KEY_BRIGHTNESSDOWN },
+	{KE_KEY, NOTIFY_BRN_MIN + 2, KEY_BRIGHTNESSUP },
 	{KE_KEY, 0x30, KEY_SWITCHVIDEOMODE },
 	{KE_KEY, 0x31, KEY_SWITCHVIDEOMODE },
 	{KE_KEY, 0x32, KEY_SWITCHVIDEOMODE },
@@ -512,11 +514,17 @@
 	return 0;
 }
 
-static void notify_brn(void)
+static int notify_brn(void)
 {
+	/* returns the *previous* brightness, or -1 */
 	struct backlight_device *bd = eeepc_backlight_device;
 	if (bd)
+	{
+		int old = bd->props.brightness;
 		bd->props.brightness = read_brightness(bd);
+		return old;
+	}
+	return -1;
 }
 
 static void eeepc_rfkill_notify(acpi_handle handle, u32 event, void *data)
@@ -558,17 +566,33 @@
 {
 	static struct key_entry *key;
 	u16 count;
+	int brn = -ENODEV;
 
 	if (!ehotk)
 		return;
 	if (event >= NOTIFY_BRN_MIN && event <= NOTIFY_BRN_MAX)
-		notify_brn();
+		brn = notify_brn();
 	count = ehotk->event_count[event % 128]++;
 	acpi_bus_generate_proc_event(ehotk->device, event, count);
 	acpi_bus_generate_netlink_event(ehotk->device->pnp.device_class,
 					dev_name(&ehotk->device->dev), event,
 					count);
 	if (ehotk->inputdev) {
+		if (brn != -ENODEV) {
+			/* brightness-change events need special
+			 * handling for conversion to key events
+			 */
+			if (brn < 0)
+				brn = event;
+			else
+				brn += NOTIFY_BRN_MIN;
+			if (event < brn)
+				event = NOTIFY_BRN_MIN; /* brightness down */
+			else if (event > brn)
+				event = NOTIFY_BRN_MIN + 2; /* ... up */
+			else
+				event = NOTIFY_BRN_MIN + 1; /* ... unchanged */
+		}
 		key = eepc_get_entry_by_scancode(event);
 		if (key) {
 			switch (key->type) {

-- 
| Darren Salt    | linux or ds at              | nr. Ashington, | Toon
| RISC OS, Linux | youmustbejoking,demon,co,uk | Northumberland | Army
| + Burn less waste. Use less packaging. Waste less.     USE FEWER RESOURCES.

Worse things happen in C.

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Corentin Chary]

On Sun, Apr 5, 2009 at 12:10 AM, Darren Salt
<linux@youmustbejoking.demon.co.uk> wrote:
> I demand that Corentin Chary may or may not have written...
>
>>>> How can brn be -2?
>>> If notify_brn() wasn't called, it will be.
>
>> Oh, yeah, I miss the if() before notify_brn()
>
> Easily missed. ;-)
>
>>>> And why -2?
>>> Because notify_brn() won't return it (and if it ever does, it needs to be
>>> fixed). (Yes, I know, "magic numbers" and all that...)
>
>> Maybe a negative known error code could be used here
>
> I see the following:
>
> * read_acpi_int() returns 0 and writes the brightness setting to *val, or
>  returns -1 and writes -1 to *val if the ACPI call failed.
>
> * get_acpi() returns whatever was written to value by read_acpi_int(), or
>  -ENODEV if there's no ACPI method which can be called.
>
> * read_brightness is a trivial wrapper for get_acpi().
>
> * notify_brn() stores the result of read_brightness and returns the
>  previously-stored value, so it can store then later return -ENODEV, -1 or
>  the brightness setting.
>
> This makes -ENODEV a suitable value. Replacing that -1 with something other
> than -ENODEV might be good, but I don't think that that really matters right
> now (though I've replaced the "!= -1" in the original version of the patch
> with "< 0").
>
> Revised patch follows...

Thanks, it's cleaner now =).

There was just a little

ERROR: that open brace { should be on the previous line
#36: FILE: drivers/platform/x86/eeepc-laptop.c:521:
        if (bd)
+       {

But I corrected that. It is now pushed in acpi4asus tree.

-- 
Corentin Chary
http://xf.iksaif.net

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Alan Jenkins]

On 4/4/09, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Fri, Apr 03, 2009 at 06:57:50PM +0100, Darren Salt wrote:
>> This maps the brightness control events to one of two keys, either
>> KEY_BRIGHTNESSDOWN or KEY_BRIGHTNESSUP, as needed.
>>
>> Some mapping has to be done due to the fact that the BIOS reports them as
>> <base value> + <current brightness index>; the selection is done according
>> to
>> the sign of the change in brightness (if this is 0, no keypress is
>> reported).
>>
>> (Ref.
>> http://lists.alioth.debian.org/pipermail/debian-eeepc-devel/2009-April/002001.html)
>>
>> Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>
>
> The reason I didn't do this is that the Eee changes the input brightness
> in hardware, which means reporting it via the input layer as well can
> cause a single keypress to raise the brightness by two steps - one in
> hardware and one triggered by userland's response to the key press. I'd
> be a little bit wary of this causing problems.
>
> On the other hand, the default behaviour of the acpi video driver is to
> change the brightness itself and then also to send the even to
> userspace, so I guess if it was going to break things it probably would
> have done already...

Actually, I think userspace has learnt to hack around it but it
doesn't work perfectly.  I would like to request that this change be
reverted, or otherwise improved.

Before this patch (2.6.29.4), gnome-power-manager doesn't interfere
with the brightness keys, and they work smoothly.

After this patch (2.6.30-rc7), g-p-m produces a "nice" popup in the
middle of my tiny netbook screen.  Unfortunately it can't be disabled,
but that's not your fault :-).  The brightness controls generally work
ok.  It doesn't jump two steps in response to one brightness keypress.
 But:

1) If I'm thrashing the SSD.  I get jerky after-effects, where g-p-m
seems to take too long to "catch up" with the brightness change.

2) If I go all the way down from full (holding down the "brightness
down" key), and then back up a few steps.  I get a noticable flash
where the brightness looks to go up two steps, then down one.  It's
probably most noticable here because the step change between the
lowest and the second lowest brightness is much more visible than any
of the other steps.

Both seem realistic use cases on this hardware.  It's obviously a
cheap SSD which is prone to latency during large writes.  And when I
move between rooms, I often adjust the brightness this way, to find
the minimum brightness which is comfortable.

Thanks
Alan

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Corentin Chary]

On Mon, Jun 8, 2009 at 5:24 PM, Alan
Jenkins<sourcejedi.lkml@googlemail.com> wrote:
> On 4/4/09, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> On Fri, Apr 03, 2009 at 06:57:50PM +0100, Darren Salt wrote:
>>> This maps the brightness control events to one of two keys, either
>>> KEY_BRIGHTNESSDOWN or KEY_BRIGHTNESSUP, as needed.
>>>
>>> Some mapping has to be done due to the fact that the BIOS reports them as
>>> <base value> + <current brightness index>; the selection is done according
>>> to
>>> the sign of the change in brightness (if this is 0, no keypress is
>>> reported).
>>>
>>> (Ref.
>>> http://lists.alioth.debian.org/pipermail/debian-eeepc-devel/2009-April/002001.html)
>>>
>>> Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>
>>
>> The reason I didn't do this is that the Eee changes the input brightness
>> in hardware, which means reporting it via the input layer as well can
>> cause a single keypress to raise the brightness by two steps - one in
>> hardware and one triggered by userland's response to the key press. I'd
>> be a little bit wary of this causing problems.
>>
>> On the other hand, the default behaviour of the acpi video driver is to
>> change the brightness itself and then also to send the even to
>> userspace, so I guess if it was going to break things it probably would
>> have done already...
>
> Actually, I think userspace has learnt to hack around it but it
> doesn't work perfectly.  I would like to request that this change be
> reverted, or otherwise improved.
>
> Before this patch (2.6.29.4), gnome-power-manager doesn't interfere
> with the brightness keys, and they work smoothly.
>
> After this patch (2.6.30-rc7), g-p-m produces a "nice" popup in the
> middle of my tiny netbook screen.  Unfortunately it can't be disabled,
> but that's not your fault :-).  The brightness controls generally work
> ok.  It doesn't jump two steps in response to one brightness keypress.
>  But:
>
> 1) If I'm thrashing the SSD.  I get jerky after-effects, where g-p-m
> seems to take too long to "catch up" with the brightness change.

There is the same "lag" problem with sound :/

> 2) If I go all the way down from full (holding down the "brightness
> down" key), and then back up a few steps.  I get a noticable flash
> where the brightness looks to go up two steps, then down one.  It's
> probably most noticable here because the step change between the
> lowest and the second lowest brightness is much more visible than any
> of the other steps.
>
I tried to install gnome-power-manager to test that, but there is no "popup".
What do I have to install to test that ? entire gnome desktop :/ ?

Thanks


-- 
Corentin Chary
http://xf.iksaif.net - http://uffs.org

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Alan Jenkins]

Corentin Chary wrote:
> On Mon, Jun 8, 2009 at 5:24 PM, Alan
> Jenkins<sourcejedi.lkml@googlemail.com> wrote:
>   
>> On 4/4/09, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>>     
>>> On Fri, Apr 03, 2009 at 06:57:50PM +0100, Darren Salt wrote:
>>>       
>>>> This maps the brightness control events to one of two keys, either
>>>> KEY_BRIGHTNESSDOWN or KEY_BRIGHTNESSUP, as needed.
>>>>
>>>> Some mapping has to be done due to the fact that the BIOS reports them as
>>>> <base value> + <current brightness index>; the selection is done according
>>>> to
>>>> the sign of the change in brightness (if this is 0, no keypress is
>>>> reported).
>>>>
>>>> (Ref.
>>>> http://lists.alioth.debian.org/pipermail/debian-eeepc-devel/2009-April/002001.html)
>>>>
>>>> Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>
>>>>         
>>> The reason I didn't do this is that the Eee changes the input brightness
>>> in hardware, which means reporting it via the input layer as well can
>>> cause a single keypress to raise the brightness by two steps - one in
>>> hardware and one triggered by userland's response to the key press. I'd
>>> be a little bit wary of this causing problems.
>>>
>>> On the other hand, the default behaviour of the acpi video driver is to
>>> change the brightness itself and then also to send the even to
>>> userspace, so I guess if it was going to break things it probably would
>>> have done already...
>>>       
>> Actually, I think userspace has learnt to hack around it but it
>> doesn't work perfectly.  I would like to request that this change be
>> reverted, or otherwise improved.
>>
>> Before this patch (2.6.29.4), gnome-power-manager doesn't interfere
>> with the brightness keys, and they work smoothly.
>>
>> After this patch (2.6.30-rc7), g-p-m produces a "nice" popup in the
>> middle of my tiny netbook screen.  Unfortunately it can't be disabled,
>> but that's not your fault :-).  The brightness controls generally work
>> ok.  It doesn't jump two steps in response to one brightness keypress.
>>  But:
>>
>> 1) If I'm thrashing the SSD.  I get jerky after-effects, where g-p-m
>> seems to take too long to "catch up" with the brightness change.
>>     
>
> There is the same "lag" problem with sound :/
>   

Yeah :/.  At it's worst, it isn't a pure "lag".  It's a bit harder to 
explain.  The firmware still changes the brightness immediately.  It 
seems that when g-p-m gets delayed, it responds _wrongly_. It doesn't 
realize that the firmware already changed the brightness, so it changes 
the brightness again.

That's why I think this is a bad "feature".  User-space is working 
around it, but the workaround isn't reliable.  I think the proper 
solution is that if userspace wants to respond to firmware-initiated 
brightness changes, it should listen for uevents on the brightness class 
device.

You can see the problem most clearly by pressing the brightness keys in 
quick succession e.g. 3 times in a row, and see 3+3 brightness changes.  
I reproduced this with

1) 2 writers:
    F=1; while true do dd if=/dev/zero of=$F bs=1M count=1 conv=sync; done &
    F=2; while true do dd if=/dev/zero of=$F bs=1M count=1 conv=sync; done &

2) 1 reader:
    dd if=/dev/sda of=/dev/null

3) Drop caches before pressing the brightness keys
    echo 1 > /sys/proc/vm/drop_caches


>> 2) If I go all the way down from full (holding down the "brightness
>> down" key), and then back up a few steps.  I get a noticable flash
>> where the brightness looks to go up two steps, then down one.  It's
>> probably most noticable here because the step change between the
>> lowest and the second lowest brightness is much more visible than any
>> of the other steps.
>>
>>     
> I tried to install gnome-power-manager to test that, but there is no "popup".
> What do I have to install to test that ? entire gnome desktop :/ ?
>
> Thanks
>   

That's weird.  I'm running it from KDE here (g-p-m package version 
2.22.1-4 on debian stable).  I'm pretty sure the only other GNOME app I 
have installed is Pidgin.  I would know if I had much more installed, 
because I'm often running short of disk space :-).

Maybe you have a newer version, that doesn't try to do such unreliable 
things?

Thanks for your time
Alan

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Corentin Chary]

On Sat, Jun 13, 2009 at 11:33 AM, Alan
Jenkins<alan-jenkins@tuffmail.co.uk> wrote:
> Corentin Chary wrote:
>>
>> On Mon, Jun 8, 2009 at 5:24 PM, Alan
>> Jenkins<sourcejedi.lkml@googlemail.com> wrote:
>>
>>>
>>> On 4/4/09, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>>>
>>>>
>>>> On Fri, Apr 03, 2009 at 06:57:50PM +0100, Darren Salt wrote:
>>>>
>>>>>
>>>>> This maps the brightness control events to one of two keys, either
>>>>> KEY_BRIGHTNESSDOWN or KEY_BRIGHTNESSUP, as needed.
>>>>>
>>>>> Some mapping has to be done due to the fact that the BIOS reports them
>>>>> as
>>>>> <base value> + <current brightness index>; the selection is done
>>>>> according
>>>>> to
>>>>> the sign of the change in brightness (if this is 0, no keypress is
>>>>> reported).
>>>>>
>>>>> (Ref.
>>>>>
>>>>> http://lists.alioth.debian.org/pipermail/debian-eeepc-devel/2009-April/002001.html)
>>>>>
>>>>> Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>
>>>>>
>>>>
>>>> The reason I didn't do this is that the Eee changes the input brightness
>>>> in hardware, which means reporting it via the input layer as well can
>>>> cause a single keypress to raise the brightness by two steps - one in
>>>> hardware and one triggered by userland's response to the key press. I'd
>>>> be a little bit wary of this causing problems.
>>>>
>>>> On the other hand, the default behaviour of the acpi video driver is to
>>>> change the brightness itself and then also to send the even to
>>>> userspace, so I guess if it was going to break things it probably would
>>>> have done already...
>>>>
>>>
>>> Actually, I think userspace has learnt to hack around it but it
>>> doesn't work perfectly.  I would like to request that this change be
>>> reverted, or otherwise improved.
>>>
>>> Before this patch (2.6.29.4), gnome-power-manager doesn't interfere
>>> with the brightness keys, and they work smoothly.
>>>
>>> After this patch (2.6.30-rc7), g-p-m produces a "nice" popup in the
>>> middle of my tiny netbook screen.  Unfortunately it can't be disabled,
>>> but that's not your fault :-).  The brightness controls generally work
>>> ok.  It doesn't jump two steps in response to one brightness keypress.
>>>  But:
>>>
>>> 1) If I'm thrashing the SSD.  I get jerky after-effects, where g-p-m
>>> seems to take too long to "catch up" with the brightness change.
>>>
>>
>> There is the same "lag" problem with sound :/
>>
>
> Yeah :/.  At it's worst, it isn't a pure "lag".  It's a bit harder to
> explain.  The firmware still changes the brightness immediately.  It seems
> that when g-p-m gets delayed, it responds _wrongly_. It doesn't realize that
> the firmware already changed the brightness, so it changes the brightness
> again.
>
> That's why I think this is a bad "feature".  User-space is working around
> it, but the workaround isn't reliable.  I think the proper solution is that
> if userspace wants to respond to firmware-initiated brightness changes, it
> should listen for uevents on the brightness class device.
>
> You can see the problem most clearly by pressing the brightness keys in
> quick succession e.g. 3 times in a row, and see 3+3 brightness changes.  I
> reproduced this with
>
> 1) 2 writers:
>   F=1; while true do dd if=/dev/zero of=$F bs=1M count=1 conv=sync; done &
>   F=2; while true do dd if=/dev/zero of=$F bs=1M count=1 conv=sync; done &
>
> 2) 1 reader:
>   dd if=/dev/sda of=/dev/null
>
> 3) Drop caches before pressing the brightness keys
>   echo 1 > /sys/proc/vm/drop_caches
>
>
>>> 2) If I go all the way down from full (holding down the "brightness
>>> down" key), and then back up a few steps.  I get a noticable flash
>>> where the brightness looks to go up two steps, then down one.  It's
>>> probably most noticable here because the step change between the
>>> lowest and the second lowest brightness is much more visible than any
>>> of the other steps.
>>>
>>>
>>
>> I tried to install gnome-power-manager to test that, but there is no
>> "popup".
>> What do I have to install to test that ? entire gnome desktop :/ ?
>>
>> Thanks
>>
>
> That's weird.  I'm running it from KDE here (g-p-m package version 2.22.1-4
> on debian stable).  I'm pretty sure the only other GNOME app I have
> installed is Pidgin.  I would know if I had much more installed, because I'm
> often running short of disk space :-).
>
> Maybe you have a newer version, that doesn't try to do such unreliable
> things?
>
> Thanks for your time
> Alan
>
Hum .. running g-p-m as root, I can get the popup, strange
Version: 2.24.2-2ubuntu8
Ok I can reproduce that.

I want to check if we can't fix g-p-m before reverting the patch. If
there is no way to fix it, I'll revert.

-- 
Corentin Chary
http://xf.iksaif.net - http://uffs.org

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Darren Salt]

I demand that Corentin Chary may or may not have written...

> On Sat, Jun 13, 2009 at 11:33 AM, Alan Jenkins<alan-jenkins@tuffmail.co.uk>
> wrote:
[snip]
>> The firmware still changes the brightness immediately.  It seems
>> that when g-p-m gets delayed, it responds _wrongly_. It doesn't realize
>> that the firmware already changed the brightness, so it changes the
>> brightness again.

Should it be changing the brightness at all? I ask because every laptop which
I've used will change the brightness without userspace being involved.
(Although it's possible that g-p-m might get brightness-change events from
some source other than that which is used to report the laptop's own
brightness controls...)

[snip]
> Version: 2.24.2-2ubuntu8
> Ok I can reproduce [the brightness being changed inappropriately from
> userspace].

> I want to check if we can't fix g-p-m before reverting the patch. If
> there is no way to fix it, I'll revert.

I don't see that it can ever be reliable in the face of the brightness having
already been changed without userspace involvement short of being able to
tell it to report only on some/all events from some input devices.

-- 
| Darren Salt      | linux at youmustbejoking | nr. Ashington, | Doon
| Debian GNU/Linux | or ds    ,demon,co,uk    | Northumberland | Army
| + Generate power using sun, wind, water, nuclear.      FORGET COAL AND OIL.

A foolish consistency is the hobgoblin of little minds.

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Alan Jenkins]

On 6/13/09, Darren Salt <linux@youmustbejoking.demon.co.uk> wrote:
> I demand that Corentin Chary may or may not have written...
>
>> On Sat, Jun 13, 2009 at 11:33 AM, Alan
>> Jenkins<alan-jenkins@tuffmail.co.uk>
>> wrote:
> [snip]
>>> The firmware still changes the brightness immediately.  It seems
>>> that when g-p-m gets delayed, it responds _wrongly_. It doesn't realize
>>> that the firmware already changed the brightness, so it changes the
>>> brightness again.
>
> Should it be changing the brightness at all? I ask because every laptop
> which
> I've used will change the brightness without userspace being involved.
> (Although it's possible that g-p-m might get brightness-change events from
> some source other than that which is used to report the laptop's own
> brightness controls...)

Good point.  Google suggests it may be necessary on some other systems though.

<https://lists.ubuntu.com/archives/kubuntu-bugs/2009-February/067008.html>

And I was wrong before when I said g-p-m should watch the generic
backlight interface.  It doesn't generate uevents, and in one way
that's good, because uevents are relatively heavyweight.  So the
brightness up / down "keypress" events are the only generic way that
g-p-m can use to detect changes :-(.

But I don't think it's important to be able to show a brightness
pop-up.  So I'm less confident, but I still think this change should
be reverted.

> [snip]
>> Version: 2.24.2-2ubuntu8
>> Ok I can reproduce [the brightness being changed inappropriately from
>> userspace].
>
>> I want to check if we can't fix g-p-m before reverting the patch. If
>> there is no way to fix it, I'll revert.
>
> I don't see that it can ever be reliable in the face of the brightness
> having
> already been changed without userspace involvement short of being able to
> tell it to report only on some/all events from some input devices.

Hmm.  I think I could accept it if it only played up when thrashing
the SSD.  So I'll try to work out exactly what happens in the other
case I reported, in case they're not the same problem.  (This other
problem is where I hold down "brightness down", then tap "brightness
up" a couple of times; the first couple of taps casue a flash as the
brightness goes first up and then down).

Alan

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Corentin Chary]

On Sat, Jun 13, 2009 at 7:51 PM, Alan
Jenkins<sourcejedi.lkml@googlemail.com> wrote:
> On 6/13/09, Darren Salt <linux@youmustbejoking.demon.co.uk> wrote:
>> I demand that Corentin Chary may or may not have written...
>>
>>> On Sat, Jun 13, 2009 at 11:33 AM, Alan
>>> Jenkins<alan-jenkins@tuffmail.co.uk>
>>> wrote:
>> [snip]
>>>> The firmware still changes the brightness immediately.  It seems
>>>> that when g-p-m gets delayed, it responds _wrongly_. It doesn't realize
>>>> that the firmware already changed the brightness, so it changes the
>>>> brightness again.
>>
>> Should it be changing the brightness at all? I ask because every laptop
>> which
>> I've used will change the brightness without userspace being involved.
>> (Although it's possible that g-p-m might get brightness-change events from
>> some source other than that which is used to report the laptop's own
>> brightness controls...)
>
> Good point.  Google suggests it may be necessary on some other systems though.
>
> <https://lists.ubuntu.com/archives/kubuntu-bugs/2009-February/067008.html>
>
> And I was wrong before when I said g-p-m should watch the generic
> backlight interface.  It doesn't generate uevents, and in one way
> that's good, because uevents are relatively heavyweight.  So the
> brightness up / down "keypress" events are the only generic way that
> g-p-m can use to detect changes :-(.
>
> But I don't think it's important to be able to show a brightness
> pop-up.  So I'm less confident, but I still think this change should
> be reverted.
>
>> [snip]
>>> Version: 2.24.2-2ubuntu8
>>> Ok I can reproduce [the brightness being changed inappropriately from
>>> userspace].
>>
>>> I want to check if we can't fix g-p-m before reverting the patch. If
>>> there is no way to fix it, I'll revert.
>>
>> I don't see that it can ever be reliable in the face of the brightness
>> having
>> already been changed without userspace involvement short of being able to
>> tell it to report only on some/all events from some input devices.
>
> Hmm.  I think I could accept it if it only played up when thrashing
> the SSD.  So I'll try to work out exactly what happens in the other
> case I reported, in case they're not the same problem.  (This other
> problem is where I hold down "brightness down", then tap "brightness
> up" a couple of times; the first couple of taps casue a flash as the
> brightness goes first up and then down).
>
> Alan
>

CCed gnome-power-manager, as it seems to be the only userspace program
concerned.
You may be able to help us here.

You can find the complet discussion here:
http://groups.google.com/group/linux.kernel/browse_thread/thread/a7bef6cffb7c2d6b/c732f616555d5180?#c732f616555d5180


-- 
Corentin Chary
http://xf.iksaif.net - http://uffs.org

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Alan Jenkins]

On 14/06/2009, Corentin Chary <corentin.chary@gmail.com> wrote:
> CCed gnome-power-manager, as it seems to be the only userspace program
> concerned.
> You may be able to help us here.
>
> You can find the complet discussion here:
> http://groups.google.com/group/linux.kernel/browse_thread/thread/a7bef6cffb7c2d6b/c732f616555d5180?#c732f616555d5180

Since this is my complaint, I'll try to summarize.


Summary: g-p-m's reaction to brightness events makes the brightness
changes less reliable
Severity: cosmetic; decrease in quality of user experience

Hardware: Asus Eee PC.
Software: linux kernel, gnome-power-manager

Last working version: linux 2.6.29.4
First broken version: linux 2.6.30

Root cause:
1) The eeepc-laptop platform driver added support for brightness
events.  They occur when the brightness up/down keys are pressed, and
are exported as normal keypresses (on a specific input device).  This
is apparently the same thing other kernel drivers do on other systems.

2) g-p-m isn't sure whether the firmware is changing the brightness
when the brightness keys are pressed.  (The EeePc firmware does change
the brightness, like most laptops).  It has a workaround for this
problem, but it isn't completely reliable.  In some cases g-p-m gets
it wrong, and changes the brightness a second time.

You can see the problem by looking at the code, and considering what
happens when more than one input event is buffered at a time:

	/* check to see if the panel has changed */
	gpm_brightness_lcd_get_hw (brightness, &current_hw);

	/* the panel has been updated in firmware */
	if (current_hw != brightness->priv->last_set_hw) {
		brightness->priv->last_set_hw = current_hw;
	} else {
		[ increment the brightness ]
	}

The first event will be ignored as expected.  But on the second event,
g-p-m will re-apply the brightness change.  It doesn't notice that the
brightness jumped _several_ steps before it processed the first event.

Symptoms:
1) When thrashing the EeePC's cheap sold-state drive, the system
becomes much slower to respond.  If you tap the brightness up key
three times, you can see the brightness jump more than 3 steps.  The
first 3 steps are immediate, then g-p-m appears to "catch up", and
erroneously re-apply the brightness changes.

This is a neat way to demonstrate the problem (see upthread for exact
reproduction steps), but we don't really care too much about it.
Laggy systems are laggy and strange.  I don't find this surprising and
I think most users will accept it, even if we could do better.

2) Switching quickly between holding "brightness down" and "brightness
up" can cause a flicker/flash of brightness.  This flash goes away
when g-p-m is killed (or on older kernels).

More specifically:
i) Set the screen to maximum brightness
ii) Hold down "brightness down"
iii) When brightness is at minimum, immediately release it and hold
down "brightness up" (or quickly tap it multiple times).

What probably happens is that g-p-m "falls behind" during step ii).
During step ii), all this does is cause the brightness to change a
little bit faster.  However, it means that in step iii), it's still
trying to decrease the brightness when the firmware starts to increase
the brightness.  So during step iii) I think you see the firmware
increase the brightness, then g-p-m decrease the brightness, and then
g-p-m catches up and the brightness increases again.

This crosses my annoyance threshold.  That's partly because it's a
regression, and because I've spent a lot of time tracking down
brightness-change related regressions which ultimately crashed the
entire system.  But I do think it's an annoyance in it's own right.
It's definitely a realistic scenario.  One will often move these small
laptops around and adjust the brightness to suit different light
conditions.

Thanks
Alan

Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Alan Jenkins]

On 6/15/09, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> On 14/06/2009, Corentin Chary <corentin.chary@gmail.com> wrote:
>> CCed gnome-power-manager, as it seems to be the only userspace program
>> concerned.

Warning pour les autres: subscription-only mailing list.

Posts on the g-p-m list will be subject to moderation and delays.

>> You may be able to help us here.
>>
>> You can find the complet discussion here:
>> http://groups.google.com/group/linux.kernel/browse_thread/thread/a7bef6cffb7c2d6b/c732f616555d5180?#c732f616555d5180
>
> Since this is my complaint, I'll try to summarize.
>
>
> Summary: g-p-m's reaction to brightness events makes the brightness
> changes less reliable
> Severity: cosmetic; decrease in quality of user experience
>
> Hardware: Asus Eee PC.
> Software: linux kernel, gnome-power-manager
>
> Last working version: linux 2.6.29.4
> First broken version: linux 2.6.30
>
> Root cause:
> 1) The eeepc-laptop platform driver added support for brightness
> events.  They occur when the brightness up/down keys are pressed, and
> are exported as normal keypresses (on a specific input device).  This
> is apparently the same thing other kernel drivers do on other systems.
>
> 2) g-p-m isn't sure whether the firmware is changing the brightness
> when the brightness keys are pressed.  (The EeePc firmware does change
> the brightness, like most laptops).  It has a workaround for this
> problem, but it isn't completely reliable.  In some cases g-p-m gets
> it wrong, and changes the brightness a second time.
>
> You can see the problem by looking at the code, and considering what
> happens when more than one input event is buffered at a time:
>
> 	/* check to see if the panel has changed */
> 	gpm_brightness_lcd_get_hw (brightness, &current_hw);
>
> 	/* the panel has been updated in firmware */
> 	if (current_hw != brightness->priv->last_set_hw) {
> 		brightness->priv->last_set_hw = current_hw;
> 	} else {
> 		[ increment the brightness ]
> 	}
>
> The first event will be ignored as expected.  But on the second event,
> g-p-m will re-apply the brightness change.  It doesn't notice that the
> brightness jumped _several_ steps before it processed the first event.
>
> Symptoms:
> 1) When thrashing the EeePC's cheap sold-state drive, the system
> becomes much slower to respond.  If you tap the brightness up key
> three times, you can see the brightness jump more than 3 steps.  The
> first 3 steps are immediate, then g-p-m appears to "catch up", and
> erroneously re-apply the brightness changes.
>
> This is a neat way to demonstrate the problem (see upthread for exact
> reproduction steps), but we don't really care too much about it.
> Laggy systems are laggy and strange.  I don't find this surprising and
> I think most users will accept it, even if we could do better.
>
> 2) Switching quickly between holding "brightness down" and "brightness
> up" can cause a flicker/flash of brightness.  This flash goes away
> when g-p-m is killed (or on older kernels).
>
> More specifically:
> i) Set the screen to maximum brightness
> ii) Hold down "brightness down"
> iii) When brightness is at minimum, immediately release it and hold
> down "brightness up" (or quickly tap it multiple times).
>
> What probably happens is that g-p-m "falls behind" during step ii).
> During step ii), all this does is cause the brightness to change a
> little bit faster.  However, it means that in step iii), it's still
> trying to decrease the brightness when the firmware starts to increase
> the brightness.  So during step iii) I think you see the firmware
> increase the brightness, then g-p-m decrease the brightness, and then
> g-p-m catches up and the brightness increases again.
>
> This crosses my annoyance threshold.  That's partly because it's a
> regression, and because I've spent a lot of time tracking down
> brightness-change related regressions which ultimately crashed the
> entire system.  But I do think it's an annoyance in it's own right.
> It's definitely a realistic scenario.  One will often move these small
> laptops around and adjust the brightness to suit different light
> conditions.
>
> Thanks
> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [gpm] [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Richard Hughes]

On Mon, Jun 15, 2009 at 9:09 AM, Alan
Jenkins<alan-jenkins@tuffmail.co.uk> wrote:
> So during step iii) I think you see the firmware
> increase the brightness, then g-p-m decrease the brightness, and then
> g-p-m catches up and the brightness increases again.

Sorry for not responding sooner, been moving house.

I don't think the kernel driver should modify the brightness itself,
as that's applying policy. Is this is left to userspace then we can
put on policy such as "don't allow brightness to be set below 30%" or
"automatically set the brightness using a ambient light sensor, and
use the brightness keys to set ambient thresholds".

Doing this policy in the driver is the wrong thing to do IMO.

Richard.

Re: [gpm] [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Alan Jenkins]

Richard Hughes wrote:
> On Mon, Jun 15, 2009 at 9:09 AM, Alan
> Jenkins<alan-jenkins@tuffmail.co.uk> wrote:
>   
>> So during step iii) I think you see the firmware
>> increase the brightness, then g-p-m decrease the brightness, and then
>> g-p-m catches up and the brightness increases again.
>>     
>
> Sorry for not responding sooner, been moving house.
>
> I don't think the kernel driver should modify the brightness itself,
> as that's applying policy. Is this is left to userspace then we can
> put on policy such as "don't allow brightness to be set below 30%" or
> "automatically set the brightness using a ambient light sensor, and
> use the brightness keys to set ambient thresholds".
>
> Doing this policy in the driver is the wrong thing to do IMO.
>
> Richard.
>   

The driver doesn't.  As far as we know, none of them do.  Firmware does.

Thanks
Alan

Re: [gpm] [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Richard Hughes]

On Tue, Jun 16, 2009 at 9:34 AM, Alan
Jenkins<alan-jenkins@tuffmail.co.uk> wrote:
> The driver doesn't.  As far as we know, none of them do.  Firmware does.

In that case there's a "laptop_panel.brightness_in_hardware" hal quirk
that turns off all the fancy stuff, and makes the GUI DTRT without
guessing.

Richard.

Re: [gpm] [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Corentin Chary]

On Tue, Jun 16, 2009 at 10:47 AM, Richard Hughes<hughsient@gmail.com> wrote:
> On Tue, Jun 16, 2009 at 9:34 AM, Alan
> Jenkins<alan-jenkins@tuffmail.co.uk> wrote:
>> The driver doesn't.  As far as we know, none of them do.  Firmware does.
>
> In that case there's a "laptop_panel.brightness_in_hardware" hal quirk
> that turns off all the fancy stuff, and makes the GUI DTRT without
> guessing.
>
> Richard.
>

Ok, so we just need to patch this file :
http://cgit.freedesktop.org/hal-info/tree/fdi/information/10freedesktop/10-laptop-panel-hardware.fdi
?
If found it /usr/share/hal/fdi/information/10freedesktop/

But I'm not sure how to do it to make it works on all Eeepc.

-- 
Corentin Chary
http://xf.iksaif.net - http://uffs.org

Re: [gpm] [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Richard Hughes]

On Tue, Jun 16, 2009 at 10:44 AM, Corentin
Chary<corentin.chary@gmail.com> wrote:
> Ok, so we just need to patch this file :
> http://cgit.freedesktop.org/hal-info/tree/fdi/information/10freedesktop/10-laptop-panel-hardware.fdi

Yes

> But I'm not sure how to do it to make it works on all Eeepc.

Well, really we want to match "classes of eeepc", so matching with
prefix might be a good idea.

Richard.

Re: [gpm] [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Alan Jenkins]

On 6/16/09, Richard Hughes <hughsient@gmail.com> wrote:
> On Tue, Jun 16, 2009 at 10:44 AM, Corentin
> Chary<corentin.chary@gmail.com> wrote:
>> Ok, so we just need to patch this file :
>> http://cgit.freedesktop.org/hal-info/tree/fdi/information/10freedesktop/10-laptop-panel-hardware.fdi
>
> Yes
>
>> But I'm not sure how to do it to make it works on all Eeepc.
>
> Well, really we want to match "classes of eeepc", so matching with
> prefix might be a good idea.
>
> Richard.

Yes please!  I see the 701 is (just added?) on the list, but this
applies to any hardware which is driven by the eeepc-laptop driver.
There is no "brightness up key" or "brightness down key" notification
recognised by the driver, only "brightness level is now equal to X".

Btw, I looked at other drivers and fujistsu-laptop is implemented in
the same way.

I guess the problem is the DMI manufacturer and product name don't
mention "EeePC".  At least on my model, the serial number starts with
"EeePC-", so we _could_ try to use that.

But I hope there's a nicer solution.  Corentin, do you think it would
be correct to apply this quirk to _all_ Asus laptops?

At the moment asus-laptop doesn't generate brightness events.  The
deprecated asus-acpi driver generates brightness events (via the old
procfs interface).   But the BR_UP/BR_DOWN events carry an absolute
brightness value; it strongly suggests that the firmware modifies the
brightness.  asus-acpi uses the brightness value to update a generic
backlight device - so we already rely on the value being correct,
otherwise g-p-m's heuristic won't work.

Thanks
Alan

Re: [gpm] [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Corentin Chary]

On Thu, Jun 18, 2009 at 3:33 PM, Alan
Jenkins<sourcejedi.lkml@googlemail.com> wrote:
> On 6/16/09, Richard Hughes <hughsient@gmail.com> wrote:
>> On Tue, Jun 16, 2009 at 10:44 AM, Corentin
>> Chary<corentin.chary@gmail.com> wrote:
>>> Ok, so we just need to patch this file :
>>> http://cgit.freedesktop.org/hal-info/tree/fdi/information/10freedesktop/10-laptop-panel-hardware.fdi
>>
>> Yes
>>
>>> But I'm not sure how to do it to make it works on all Eeepc.
>>
>> Well, really we want to match "classes of eeepc", so matching with
>> prefix might be a good idea.
>>
>> Richard.
>
> Yes please!  I see the 701 is (just added?) on the list, but this

The 701 was already here when I checked.

> But I hope there's a nicer solution.  Corentin, do you think it would
> be correct to apply this quirk to _all_ Asus laptops?

I can confirm that all the asus laptop I know change the brightness in the
firmware. So, in my opinion, we can safely apply this quirk to all Asus laptops.


-- 
Corentin Chary
http://xf.iksaif.net - http://uffs.org